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.

Sign up for GitHub

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

Jump to bottom

Reduce usage of RequirementSet in commands #7704

Merged
merged 12 commits into from
Feb 6, 2020
Merged

Reduce usage of RequirementSet in commands #7704

merged 12 commits into from
Feb 6, 2020

Conversation

chrahunt
Copy link
Member

@chrahunt chrahunt commented Feb 6, 2020 •
edited
Loading

Previously we were using a RequirementSet to hold all of our parsed InstallRequirements, passing it to Resolver to mutate, and then reading from the updated RequirementSet to get our installed/downloaded InstallRequirements.

Now we only expose RequirementSet as the result of Resolver.resolve, and hide it as an implementation detail for holding parsed InstallRequirements. This means when implementing the new resolver we now only need to worry about translating InstallRequirements before invoking it and not both InstallRequirements and a RequirementSet.

Fixes #7571 and fixes #7638.

@chrahunt
Remove old comment from Resolver.resolve
441b211 
This was moved to RequirementCommand.populate_requirement_set, so it's
no longer applicable in this context.
@chrahunt chrahunt added type: refactor Refactoring code skip news Does not need a NEWS file entry (eg: trivial changes) labels Feb 6, 2020
@chrahunt chrahunt changed the title Refactor/simpler resolver interface Reduce usage of RequirementSet in commands Feb 6, 2020
chrahunt added 11 commits February 5, 2020 22:29
@chrahunt
Return a different RequirementSet from Resolver.resolve
58c0629 
This makes the resolver interface simpler by returning a brand new
RequirementSet vs mutating the one that was input to the function, and
will let us specialize RequirementSet for the different use cases.
@chrahunt
Parameterize check_supported_wheels to Resolver.resolve
550ae90 
This reduces our dependence on the input RequirementSet.
@chrahunt
Centralize logic for getting 'all requirements'
a814dc9
@chrahunt
Pass in a plain list of InstallRequirement to Resolver.resolve
fd815bc 
Further simplifies the Resolver interface, and will give us the
opportunity to remove any knowledge of RequirementSet from the
individual commands.
@chrahunt
Return a list of InstallRequirement from populate_requirement_set
5dcc562 
Next we can hide RequirementSet from the setup phase of the individual
commands.
@chrahunt
Inline check_supported_wheels
779d8df 
Reduces our dependency on RequirementSet in individual commands.

The default value for this is True, used everywhere except
InstallCommand.
@chrahunt
Do not use InstallRequirement in warn_deprecated_install_options
9c58aa7 
This only needed a list of requirements, so give it just that.
@chrahunt
Make new RequirementSet in populate_requirement_set
e872d2b 
Concentrates use of RequirementSet in individual commands so we can
eventually break it up.
@chrahunt
Remove unused requirement_set argument
de5ec7e
@chrahunt
Remove unused requirement_set in commands
543c844
@chrahunt
Rename populate_requirement_set to get_requirements
e7998a3
@chrahunt chrahunt force-pushed the refactor/simpler-resolver-interface branch from 0b36b7a to e7998a3 Compare February 6, 2020 03:30
@chrahunt chrahunt marked this pull request as ready for review February 6, 2020 05:14
pradyunsg
pradyunsg requested changes Feb 6, 2020
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

LGTM barring one comment regarding the resolve method.

TIL, code >> text, when communicating ideas for a refactor. :)

src/pip/_internal/legacy_resolve.py Show resolved Hide resolved
@chrahunt chrahunt mentioned this pull request Feb 6, 2020
Closed
@xavfernandez xavfernandez merged commit 8b20443 into pypa:master Feb 6, 2020
@pfmoore
Copy link
Member

pfmoore commented Feb 7, 2020

As a followup, with the sole exception of the successfully_downloaded reference in pip._internal.commands.download (which could be easily fixed, I suspect) we could actually just return a list of Requirements from resolve, by returning the output of resolver.get_installation_order(). It does a bit of extra work in cases where we don't need the ordering, but the saving is that RequirementSet becomes entirely an internal detail of the legacy resolver.

Would this be worth doing? I have a half-prepared PR which I can finish if it seems worthwhile.

@pradyunsg
Copy link
Member

It is worth doing, yea.

I'm not sure what your PR looks like, but it might make sense to have separate PRs to get rid of successfully_downloaded, and then stop returning requirementset.

(sorry for typos/weird capitalization, on mobile)

@pfmoore
Copy link
Member

pfmoore commented Feb 7, 2020

See #7710 for the first step (moving the successfully_downloaded logic). I'll do the follow-up to return a list of requirements once that is merged, to avoid having to deal with merge conflicts, which scare me :-)

@chrahunt chrahunt deleted the refactor/simpler-resolver-interface branch February 8, 2020 04:14
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Mar 10, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pradyunsg pradyunsg pradyunsg approved these changes

@xavfernandez xavfernandez xavfernandez approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes) type: refactor Refactoring code
Projects
None yet
Milestone
No milestone
4 participants
@chrahunt @pfmoore @pradyunsg @xavfernandez