Move prepare_files logic to pip.resolve #4526

pradyunsg · 2017-05-31T18:30:20Z

This is the first of my GSoC 2017 PRs! 🎉

This PR moves out all the logic in pip.req.req_set.RequirementSet.prepare_files into a new pip.resolve module. The idea is that all the things that are to be done while determining which packages (and versions) should be installed would happen in this new module.

I'm making this PR so that I can have tests run on the Travis CI - which happens to be faster than on my laptop and also frees my laptop up so that I can continue working. :)

TODO:

Move the code
Get the tests passing
~~Move parts of RequirementSet.__init__ to pip.resolve.Resolver~~ (will do in next PR)

PS: This feels a lot like taking the heart out of a beast. XD

Move out code (pip.req.req_set.prepare_files and support) out into pip.resolve

Use a method to prevent duplication across install and download

This likely means that even the tests would need to move around... That's for later though.

xavfernandez

This seems like a good direction but please, keep the PRs smalls (hopefully that's what you had in mind).
Ideally, one PR to refactor and an other PR to improve/change things.

xavfernandez · 2017-05-31T21:02:40Z

pip/basecommand.py

+
+        return Resolver(
+            upgrade_strategy=strategy,
+            finder=finder


nit: final ,

pradyunsg · 2017-06-01T05:04:13Z

@xavfernandez That's exactly what I have in mind.

In this PR, I am only going to move (a fair bit of) code and change only things that are needed to make it pass the tests. Once that is done, I'll make another PR actually making an improvement on the functionality or behaviour.

The only exception in this PR to this rule, right now, is upgrade_strategy handling. Should I make those changes - special handling and moving attributes of RequirementSet - in the next PR, not this one?

pypa-bot · 2017-06-01T05:12:16Z

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

xavfernandez · 2017-06-01T08:55:53Z

The only exception in this PR to this rule, right now, is upgrade_strategy handling. Should I make those changes - special handling and moving attributes of RequirementSet - in the next PR, not this one?

Well, as long as you set the strategy to not-allowed for download and wheel commands I think it can stay in this PR.

pradyunsg · 2017-06-01T13:24:16Z

Well, as long as you set the strategy to not-allowed for download and wheel commands I think it can stay in this PR.

That's how it is. :)

pradyunsg · 2017-06-06T18:23:13Z

Umm... I can't find what's causing these tests to fail. I'm pretty sure it's some line I messed up while updating for the change in location but I can not seem to pin point it.

@dstufft @xavfernandez @pfmoore Could one of you please look into this?

PS: Starting today, I'll be afk for around a week. :/ Sorry.

pradyunsg · 2017-06-13T04:15:17Z

pip/resolve.py

+                    req_to_install.check_if_exists()
+                if req_to_install.satisfied_by:
+                    should_modify = (
+                        self.upgrade_strategy == "not-allowed" and


This should be an or!!!

Hopefully, this is all that needs fixing.

pradyunsg · 2017-06-13T06:01:36Z

/request-review @dstufft @xavfernandez @pfmoore

dstufft · 2017-06-13T17:35:52Z

pip/commands/download.py

@@ -196,7 +196,8 @@ def run(self, options, args):
                    None
                )

-                requirement_set.prepare_files(finder)
+                resolver = self._build_resolver(options, finder)
+                resolver.resolve(requirement_set)


It appears like the way this is working is to mutate the RequirementSet that we're passing in. That is OK for this PR since that more closely resembles the existing code (and thus reduces the amount of code churn), but thinking forward I think it would be great if we can get this code refactored so we're passing in the things we want to install, and then it returns a new thing that lists everything we need to install. These could both be RequirementSetss possibly?

Anyways, not an immediate concern, just something to keep in mind.

These could both be RequirementSets possibly?

Is exactly what I'm thinking.

dstufft · 2017-06-13T17:38:30Z

pip/req/req_set.py

+            not self.requirements[name].constraint or
+            name in self.requirement_aliases and
+            not self.requirements[self.requirement_aliases[name]].constraint
+        )


This looks like it was just reformatting? If so can we just revert this back to the existing code. A separate PR could clean this up, but atm I think it's just adding noise to an already large diff.

Roger that!

dstufft · 2017-06-13T17:41:09Z

Did we just not have any tests for this functionality before? Or was it all tested indirectly by calling commands that used prepare_files?

pradyunsg · 2017-06-13T17:44:20Z

Did we just not have any tests for this functionality before? Or was it all tested indirectly by calling commands that used prepare_files?

It seems to me that it's the latter. A whole bunch of tests broke when I made a change that changed the behavior from that of prepare_files.

dstufft · 2017-06-13T17:46:56Z

It seems to me that it's the latter. A whole bunch of tests broke when I made a change that changed the behavior from that of prepare_files.

Ok good, at least we have some validation that the this isn't breaking things! At some level it's more or less impossible to actually do a meaningful code review on a large copy/paste job like this. Given the tests were not (really) modified I'm inclined to believe the copy/paste was done OK and once the minor nits I pointed out get addressed I'm happy to merge this.

pradyunsg added 13 commits May 31, 2017 10:17

Remove an useless if

cf0212d

Add pip.resolve

8e4b226

Move out code (pip.req.req_set.prepare_files and support) out into pip.resolve

Start using the resolver

256f67e

Take an upgrade-strategy as initialization argument

3a1fbc5

🎨

a49a79e

Substitute self with requirement_set where appropriate

7981f74

📝

d6d26a5

Make finder an initialization argument

a2447ff

Pass requirement_set to _check_skip_installed

c5a5121

Build the resolver in basecommand

4e70d96

Use a method to prevent duplication across install and download

Don't resolve inside WheelBuilder

60f4a95

Stop monkeypatching the wrong thing in the tests

c18ee34

This likely means that even the tests would need to move around... That's for later though.

I forgot that download doesn't have an upgrade_strategy.

edd3c36

xavfernandez reviewed May 31, 2017

View reviewed changes

pip/basecommand.py Outdated

return Resolver(

upgrade_strategy=strategy,

finder=finder

Copy link

Member

xavfernandez May 31, 2017

Choose a reason for hiding this comment

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

nit: final ,

pypa-bot added the needs rebase or merge PR has conflicts with current master label Jun 1, 2017

pradyunsg and others added 3 commits June 1, 2017 18:27

🎨

0dee344

Merge branch 'pradyunsg/master' into refactor/resolve

f4dd076

Merge branch 'master' into refactor/resolve

d3bcbbd

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jun 1, 2017

pradyunsg added 3 commits June 1, 2017 18:56

🎨 unused imports

5ed8967

Pull in remote changes

7424218

Even upgrade is not available in download or wheel

2fc4904

pradyunsg commented Jun 13, 2017

View reviewed changes

pradyunsg force-pushed the refactor/resolve branch from 9a35c34 to 2bd5714 Compare June 13, 2017 04:20

🔧 Fix tests

2bd5714

Hopefully, this is all that needs fixing.

Run resolver in wheel command

38e7490

pradyunsg changed the title ~~[WIP] Move prepare_files logic to pip.resolve~~ Move prepare_files logic to pip.resolve Jun 13, 2017

📰

5e6be8b

dstufft requested changes Jun 13, 2017

View reviewed changes

Undo un-necessary reformatting

e3fd903

dstufft approved these changes Jun 13, 2017

View reviewed changes

dstufft merged commit f555802 into pypa:master Jun 13, 2017

pradyunsg deleted the refactor/resolve branch June 13, 2017 18:42

pradyunsg mentioned this pull request Jun 15, 2017

From RequirementSet to Resolver - Part 1 #4546

Merged

pradyunsg added the type: refactor Refactoring code label Oct 25, 2017

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move prepare_files logic to pip.resolve #4526

Move prepare_files logic to pip.resolve #4526

pradyunsg commented May 31, 2017 •

edited

xavfernandez left a comment

xavfernandez May 31, 2017

pradyunsg commented Jun 1, 2017

pypa-bot commented Jun 1, 2017

xavfernandez commented Jun 1, 2017

pradyunsg commented Jun 1, 2017

pradyunsg commented Jun 6, 2017 •

edited

pradyunsg Jun 13, 2017

pradyunsg commented Jun 13, 2017

dstufft Jun 13, 2017

pradyunsg Jun 13, 2017

dstufft Jun 13, 2017

pradyunsg Jun 13, 2017

dstufft commented Jun 13, 2017

pradyunsg commented Jun 13, 2017

dstufft commented Jun 13, 2017

Move prepare_files logic to pip.resolve #4526

Move prepare_files logic to pip.resolve #4526

Conversation

pradyunsg commented May 31, 2017 • edited

xavfernandez left a comment

Choose a reason for hiding this comment

xavfernandez May 31, 2017

Choose a reason for hiding this comment

pradyunsg commented Jun 1, 2017

pypa-bot commented Jun 1, 2017

xavfernandez commented Jun 1, 2017

pradyunsg commented Jun 1, 2017

pradyunsg commented Jun 6, 2017 • edited

pradyunsg Jun 13, 2017

Choose a reason for hiding this comment

pradyunsg commented Jun 13, 2017

dstufft Jun 13, 2017

Choose a reason for hiding this comment

pradyunsg Jun 13, 2017

Choose a reason for hiding this comment

dstufft Jun 13, 2017

Choose a reason for hiding this comment

pradyunsg Jun 13, 2017

Choose a reason for hiding this comment

dstufft commented Jun 13, 2017

pradyunsg commented Jun 13, 2017

dstufft commented Jun 13, 2017

pradyunsg commented May 31, 2017 •

edited

pradyunsg commented Jun 6, 2017 •

edited