Full implementation of `--keep-outdated` #3304

techalchemy · 2018-11-26T05:41:00Z

Introduces both a PEEP and a fully functional implementation of most features discussed

Arbitrarily numbers the PEEP 005 in case of merges ahead of this one
Outlines --keep-outdated behavior and provides a full working implementation
Follows implementation discussed in the initial design discussions

- Fixes #3244 Signed-off-by: Dan Ryan <[email protected]>

Signed-off-by: Dan Ryan <[email protected]>

jxltom · 2018-11-27T15:30:39Z

This is an exciting feature. If this can be merged, let's update docs reflecting the changes and we also should add tests. :D

jxltom · 2018-11-28T02:44:37Z

It looks like it is full support for pipenv install --keep-outdated but not for pipenv update --keep-outdated?

Existing packages in the project Pipfile.lock should remain in place, even if they are not returned during resolution;

This does not work perfectly for update. For example, if package a has dependencies b, c in existed pipfile.lock. And newer version a only has dependency b, with pipenv update a --keep-outdated, c should be removed if c is not dependency of any other packages in pipfile even it is not returned in resolution.

I think probably it is better to add one more step for --keep-outdated which checks dependencies of specific package in existed pipfile.lock and removes them if they are the only dependencies of that certain package and they are not returned in the new resolution.

techalchemy · 2018-11-28T14:04:20Z

That sounds like maybe we want it for --selective-upgrade but I didn’t implement this in this way by accident. One of the first things I mentioned in the PEEP is wanting a way to ensure we aren’t making any changes to the lockfile that aren’t directly caused by the upgrade, including removing stuff just because we think you don’t need it. In fact that’s a large motivation behind making this PR: anytime I upgrade a cross platform Pipfile, I have to manually undo all of the removals of platform specific dependencies which are ‘no longer needed’. This feature would be pointless if it doesn’t offer that.

jxltom · 2018-11-28T14:33:30Z

--keep-outdated is a way to ensure only direct changes can be made to pipfile.lock for install/lock/update. This makes sense to me. It would be nice for that to be documented, explaining the basic principles behind the scenes, probably as well as the differences with --selective-upgrade.

If I understand correclty, one more thing with such kind of design is that pipenv update <dep> --keep-outdated and pipenv lock --keep-outdated may keep more and more unnecessary dependencies as above example shows since these deps are actually no longer needed but being kept via --keep-outdated. It would be nice to document the side effect of this kind of usage. The pipenv install <dep> --keep-outdated doesn't have this side-effect as it will bring new deps only.

techalchemy · 2018-11-29T04:00:22Z

It's not documented because it's not implemented, the documentation is outlined in the discussion linked in the PEEP.

The only additional element I am adding in this PEEP is that --keep-outdated also keeps dependencies that pipenv deems outdated at the project level, i.e. dependencies that are not considered necessary anymore. If you want to rewrite your lockfile, pipenv lock should handle that.

It is possible we should think of another flag to use to say "I don't mind updating the dependencies I have, but don't remove anything if it's in my lockfile". I'm not totally sure what that flag would be.

pipenv/environment.py

pipenv/resolver.py

…nto feature/keep-outdated-peep

Signed-off-by: Dan Ryan <[email protected]>

ncoghlan · 2019-01-21T06:07:48Z

This looks good to me, so the main things would be test and documentation updates.

Signed-off-by: Dan Ryan <[email protected]>

- Bring in changes from bugfix/3148 Signed-off-by: Dan Ryan <[email protected]>

Signed-off-by: Dan Ryan <[email protected]>

- Lost numerous changes in the rebase, this brings them back - Should work for both sets of fixes now Signed-off-by: Dan Ryan <[email protected]>

Signed-off-by: Dan Ryan <[email protected]>

techalchemy · 2019-03-10T18:57:50Z

/cc @ncoghlan on the tests

Signed-off-by: Dan Ryan <[email protected]>

ncoghlan

Yep, those new tests look good to me - good idea to check the behaviour when the Pipfile change is to remove a pin rather than adding a new dependency.

ncoghlan · 2019-03-17T03:35:13Z

peeps/PEEP-005.md

+
+## Necessary Changes
+
+In order to make these changes, we will need to modify the dependency resolution process. Overall, locking will require the following implementaiton changes:


Typo in "implementaiton"

ncoghlan · 2019-03-17T03:35:57Z

pipenv/core.py

-            if dep.is_vcs or dep.editable:
-                c.block()
+            # if dep.is_vcs or dep.editable:
+            #     c.block()


If this genuinely isn't needed any more, better to just delete it than comment it out

ncoghlan · 2019-03-17T03:37:40Z

tests/integration/test_lock.py

@@ -92,6 +93,55 @@ def test_lock_keep_outdated(PipenvInstance, pypi):
        assert lock['default']['pytest']['version'] == "==3.1.0"


+@pytest.mark.keep_outdated
+@pytest.mark.lock


Minor cosmetic thing: since the order to the marks doesn't matter, shorter-first looks tidier.

Signed-off-by: Dan Ryan <[email protected]>

techalchemy · 2019-03-18T04:56:35Z

Pin removal is one of the things I added a test for at the end: check out

pipenv/tests/integration/test_lock.py

Lines 129 to 139 in 0b5826e

    
           p._pipfile.add("requests", "==2.18.4") 
        
           c = p.pipenv("install") 
        
           assert c.ok 
        
           p._pipfile.add("requests", "*") 
        
           assert p.pipfile["packages"]["requests"] == "*" 
        
           c = p.pipenv("lock --keep-outdated") 
        
           assert c.ok 
        
           assert "requests" in p.lockfile["default"] 
        
           assert "urllib3" in p.lockfile["default"] 
        
           # ensure this didn't update requests 
        
           assert p.lockfile["default"]["requests"]["version"] == "==2.18.4"

which essentially just --

Installs requests==2.18.4
Unpins requests and re-locks with --keep-outdated
Validates that the version of requests locked is still 2.18.4 since it satisfies the constraints
Re-locks without --keep-outdated to verify that we actually would get an updated version if we didn't hold it back

Is that what you had in mind?

ncoghlan · 2019-03-21T14:29:00Z

@techalchemy Sorry, I was overly terse in my review summary, which made it ambiguous - it was meant to mean "that was a good idea to ..." (i.e. praising the inclusion of that particular test, since it covered a case that hadn't occurred to me), but could be read as "it would be a good idea to ..." (which would have meant that I had missed the fact that the test was already part of the PR).

techalchemy · 2019-03-25T05:04:08Z

hah, no worries, thanks for the reviews!

techalchemy · 2019-03-29T23:17:00Z

@uranusjr @ncoghlan my only concern here is that the command now does too much from a design standpoint -- as a user of the tool myself, I personally would want to separate the functionality because as it stands pipenv lock --keep-outdated will:

Retain dependencies that are no longer part of the resolution graph; and,
Hold back any satisfied pins

I'll probably merge this as is for release but I want to continue thinking about splitting this up

Turbo87 · 2019-04-26T06:28:08Z

nice work on this @techalchemy! do you know when the next release for pipenv is planned? would love to try this feature out :)

techalchemy added 7 commits November 21, 2018 13:20

Fix accidentally-shortened installation timeouts

566e1c1

- Fixes #3244 Signed-off-by: Dan Ryan <[email protected]>

Create sample implementation for keep outdated functionality

052c5b8

Signed-off-by: Dan Ryan <[email protected]>

Add PEEP to outline behavior

fb99f71

Signed-off-by: Dan Ryan <[email protected]>

Merge branch 'master' into feature/keep-outdated-peep

d3bcd83

Fully functional --keep-outdated implementation

5b8b10d

Signed-off-by: Dan Ryan <[email protected]>

PEEP update and resolver fix

97962d9

Signed-off-by: Dan Ryan <[email protected]>

Merge branch 'master' into feature/keep-outdated-peep

76a5e29

techalchemy added the Status: Requires Approval This issue requires additional approval to move forward. label Nov 26, 2018

Merge branch 'master' into feature/keep-outdated-peep

de24a5f

jxltom reviewed Dec 3, 2018

View reviewed changes

pipenv/environment.py Show resolved Hide resolved

pipenv/resolver.py Outdated Show resolved Hide resolved

greysteil mentioned this pull request Dec 3, 2018

Pipenv updates pinned packages in Pipfile.lock when there is an editable git dependency with unpinned sub-deps dependabot/dependabot-core#805

Closed

techalchemy added 3 commits December 4, 2018 01:03

Merge branch 'master' into feature/keep-outdated-peep

de20227

Merge branch 'feature/keep-outdated-peep' of github.com:pypa/pipenv i…

83f5791

…nto feature/keep-outdated-peep

Bugfixes for set resolution

df712cc

Signed-off-by: Dan Ryan <[email protected]>

techalchemy mentioned this pull request Dec 7, 2018

install's --keep-outdated does not seem to be respected #1554

Closed

jxltom mentioned this pull request Jan 19, 2019

Confusion: --keep-outdated --selective-upgrade #3461

Closed

Merge branch 'master' into feature/keep-outdated-peep

3feeaa7

techalchemy added 7 commits February 17, 2019 13:39

Merge with updates

552d127

Signed-off-by: Dan Ryan <[email protected]>

Port over changes that were overwritten

698dfb8

- Bring in changes from bugfix/3148 Signed-off-by: Dan Ryan <[email protected]>

Update utils to work with patch

a33ad9d

Signed-off-by: Dan Ryan <[email protected]>

Re-integrate changes from keep_outdated

1fcafb8

- Lost numerous changes in the rebase, this brings them back - Should work for both sets of fixes now Signed-off-by: Dan Ryan <[email protected]>

add updated requirementslib

9c0bac1

Signed-off-by: Dan Ryan <[email protected]>

Update lockfile

7798cbe

Signed-off-by: Dan Ryan <[email protected]>

Update setup.py to keep pytest pinned below 4.0

fae1dba

Signed-off-by: Dan Ryan <[email protected]>

techalchemy added 2 commits March 10, 2019 04:25

Rename peep to 004

0cc3e25

Signed-off-by: Dan Ryan <[email protected]>

Merge branch 'master' into feature/keep-outdated-peep

0382e0c

techalchemy added 3 commits March 10, 2019 16:04

Rename peep to 005 to make room for kenneth's peep

3131e2f

Signed-off-by: Dan Ryan <[email protected]>

Merge branch 'master' into feature/keep-outdated-peep

93019f3

Merge branch 'master' into feature/keep-outdated-peep

83b1f12

ncoghlan changed the title ~~Full implemenation of --keep-outdated~~ Full implementation of --keep-outdated Mar 17, 2019

ncoghlan approved these changes Mar 17, 2019

View reviewed changes

techalchemy added 3 commits March 18, 2019 00:43

Merge branch 'master' into feature/keep-outdated-peep

6382f51

Signed-off-by: Dan Ryan <[email protected]>

sort pytest markers by length

e16608c

Signed-off-by: Dan Ryan <[email protected]>

Fix typo in PEEP

0b5826e

Signed-off-by: Dan Ryan <[email protected]>

techalchemy added 2 commits March 25, 2019 01:05

Merge branch 'master' into feature/keep-outdated-peep

c7b2a52

Merge branch 'master' into feature/keep-outdated-peep

ea9129a

Merge branch 'master' into feature/keep-outdated-peep

1b84c6b

techalchemy merged commit b03a594 into master Mar 30, 2019

techalchemy deleted the feature/keep-outdated-peep branch March 30, 2019 00:49

greysteil mentioned this pull request Apr 9, 2019

[Security] Build(deps): Bump jinja2 from 2.10 to 2.10.1 skylines-project/skylines#1412

Merged

greysteil mentioned this pull request May 7, 2019

Pipfile.lock getting clobbered with path based dependencies dependabot/feedback#460

Closed

redshiftzero mentioned this pull request May 14, 2019

Remove pipenv in favor of pip-tools freedomofpress/securedrop-client#370

Closed

acdha mentioned this pull request May 15, 2019

Update cfn-lint LibraryOfCongress/concordia#967

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full implementation of `--keep-outdated` #3304

Full implementation of `--keep-outdated` #3304

techalchemy commented Nov 26, 2018

jxltom commented Nov 27, 2018

jxltom commented Nov 28, 2018

techalchemy commented Nov 28, 2018

jxltom commented Nov 28, 2018 •

edited

Loading

techalchemy commented Nov 29, 2018

ncoghlan commented Jan 21, 2019

techalchemy commented Mar 10, 2019

ncoghlan left a comment

ncoghlan Mar 17, 2019

ncoghlan Mar 17, 2019

ncoghlan Mar 17, 2019

techalchemy commented Mar 18, 2019

ncoghlan commented Mar 21, 2019

techalchemy commented Mar 25, 2019

techalchemy commented Mar 29, 2019

Turbo87 commented Apr 26, 2019


		## Necessary Changes

		In order to make these changes, we will need to modify the dependency resolution process. Overall, locking will require the following implementaiton changes:

Full implementation of --keep-outdated #3304

Full implementation of --keep-outdated #3304

Conversation

techalchemy commented Nov 26, 2018

jxltom commented Nov 27, 2018

jxltom commented Nov 28, 2018

techalchemy commented Nov 28, 2018

jxltom commented Nov 28, 2018 • edited Loading

techalchemy commented Nov 29, 2018

ncoghlan commented Jan 21, 2019

techalchemy commented Mar 10, 2019

ncoghlan left a comment

Choose a reason for hiding this comment

ncoghlan Mar 17, 2019

Choose a reason for hiding this comment

ncoghlan Mar 17, 2019

Choose a reason for hiding this comment

ncoghlan Mar 17, 2019

Choose a reason for hiding this comment

techalchemy commented Mar 18, 2019

ncoghlan commented Mar 21, 2019

techalchemy commented Mar 25, 2019

techalchemy commented Mar 29, 2019

Turbo87 commented Apr 26, 2019

Full implementation of `--keep-outdated` #3304

Full implementation of `--keep-outdated` #3304

jxltom commented Nov 28, 2018 •

edited

Loading