Implement PEP 376 REQUESTED #8026

sbidoul · 2020-04-12T10:45:39Z

closes #7811
closes #8242

TODO

integration tests

sbidoul · 2020-04-12T11:07:12Z

InstallRequirement has a is_direct flag that seems to be quite close to meaning "top level requirement".

is_direct is currently only used in the legacy resolver in relation to the upgrade strategy here:

pip/src/pip/_internal/resolution/legacy/resolver.py

Lines 196 to 197 in 62c822d

    
           assert self.upgrade_strategy == "only-if-needed" 
        
           return req.is_direct

What I don't understand is why is_direct is set to True also for constraints here.

Can anyone shed some light on this matter?

sbidoul · 2020-04-12T17:20:15Z

So I set is_direct = False for constraints, so is_direct really means top level requirement, and I return req.is_direct or req.constraint in _is_upgrade_allowed(). This should be equivalent.

uranusjr · 2020-04-12T19:17:59Z

I think the original intention of is_direct is to indicate an InstallRequirement being specified by the user. In that sense constraints should set is_direct=True because they are user-specified by definition.

These meanings don’t really matter for REQUESTED though. InstallRequirement.install() is only called if constraint is false¹, so the Record the REQUESTED file block should never be reached is an InstallRequirement is a constraint. I would stick an assert at the beginning of InstallRequirement.install(), and avoid changing is_direct logic.

Note 1: The method’s only caller is install_given_reqs(), which in turn is only called only once in InstallRequirement.run(). The to_install list it receives is generated by get_installation_order() of the (legacy) resolver, which only returns an InstallRequirement is it is not a constraint.

sbidoul · 2020-04-12T20:58:31Z

@uranusjr thanks for the analysis. Actually it does matter though, because constraints are added first in the requirements set and then updated with constraint=False when a corresponding requirement to install is found. If they are marked as is_direct=True we end up generating REQUESTED for them (test_install_requested_reqs_and_constraints exercises that).

pradyunsg · 2020-04-12T23:20:09Z

I don't see any particular reason to "rush" this for pip 20.1; so I'm gonna say this isn't gonna be on the pip 20.1 train.

uranusjr · 2020-04-13T05:23:56Z

@sbidoul Thanks, it definitely makes much more sense to change the meaning of is_direct considering the information.

Maybe we should also take this chance to change the attribute name (maybe to something like is_user_supplied_requirement), and add a comment to explain the various caveats (what qualifies as user-supplied, contraints should set this to false, etc.)

BrownTruck · 2020-05-05T16:30:05Z

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 be eligible for code review and hopefully merging!

pradyunsg · 2020-05-15T13:42:18Z

src/pip/_internal/cli/req_command.py

@@ -310,15 +310,15 @@ def get_requirements(
                    parsed_req,
                    isolated=options.isolated_mode,
                )
-                req_to_add.is_direct = True
+                req_to_add.user_supplied = False


IIUC, this is intentional, to avoid constraint = True and user_supplied = True, correct?

Yes that's because the (legacy) resolver sometimes flips the constraint flag to False and then we would have something that is merely a constraints that has the REQUESTED file.

sbidoul · 2020-05-15T14:29:28Z

I rebased. The new tests for REQUESTED fail with the new resolver, though. Could it be it loses the user_supplied flag on the way?

sbidoul · 2020-05-16T09:57:15Z

Now test pass with the new resolver too. This should be good to go now.

pfmoore

I'm still not convinced of the value of the REQUESTED file in the first place, but whatever, it's part of the PEP so that's fine.

Other than a couple of small style nits, this LGTM.

src/pip/_internal/req/req_set.py

BrownTruck · 2020-05-28T22:00:02Z

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 be eligible for code review and hopefully merging!

sbidoul · 2020-05-29T20:22:34Z

I rebased to handle the conflict following the parent -> template renaming.

I guess this is good to go, if still green.

uranusjr · 2020-05-29T23:12:47Z

tests/unit/test_wheel.py

@@ -278,7 +278,7 @@ def test_std_install(self, data, tmpdir):
            scheme=self.scheme,
            req_description=str(self.req),
        )
-        self.assert_installed(0o644)
+        self.assert_installed()


Nit, I would favour reverting this since the implicit 0o644 is not obvious IMO.

@uranusjr I think it reads better with the default, since here (as well as in the new test added in this PR) we don't want to test permissions. Only in the specific test that checks for permission, we pass a specific expected_permissions.

I’m not following. The new implementation still implicitly checks the permission against 0o644, doesn’t it? The proposed change seems to make the fact less obviously than it was to me.

Ok I reverted that bit.

pradyunsg · 2020-06-24T13:21:43Z

Thanks for doing this @sbidoul! ^>^

sbidoul force-pushed the requested-sbi branch from 14be8dc to 7e6303a Compare April 12, 2020 10:48

sbidoul force-pushed the requested-sbi branch 2 times, most recently from 8176019 to 8bc9185 Compare April 12, 2020 17:17

sbidoul marked this pull request as ready for review April 12, 2020 17:25

BrownTruck added the needs rebase or merge PR has conflicts with current master label May 5, 2020

This was referenced May 13, 2020

New resolver: “Nameless” constraint #8210

Closed

Rename InstallRequirement is_direct to user_specified #8242

Closed

sbidoul force-pushed the requested-sbi branch from 6e6f150 to eb36dc7 Compare May 15, 2020 13:40

pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 15, 2020

pradyunsg approved these changes May 15, 2020

View reviewed changes

sbidoul force-pushed the requested-sbi branch from eb36dc7 to f08ce86 Compare May 15, 2020 14:05

This comment has been minimized.

Sign in to view

pypa-bot added the needs rebase or merge PR has conflicts with current master label May 16, 2020

sbidoul force-pushed the requested-sbi branch from 1004f39 to f113d45 Compare May 16, 2020 09:22

pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 16, 2020

sbidoul force-pushed the requested-sbi branch 3 times, most recently from 179d30d to 5d5fc74 Compare May 16, 2020 09:39

pfmoore approved these changes May 16, 2020

View reviewed changes

src/pip/_internal/req/req_set.py Outdated Show resolved Hide resolved

src/pip/_internal/req/req_set.py Outdated Show resolved Hide resolved

sbidoul force-pushed the requested-sbi branch from d542235 to 609e3cd Compare May 16, 2020 11:15

pradyunsg approved these changes May 16, 2020

View reviewed changes

sbidoul force-pushed the requested-sbi branch from 609e3cd to 9384055 Compare May 21, 2020 15:37

BrownTruck added the needs rebase or merge PR has conflicts with current master label May 28, 2020

sbidoul force-pushed the requested-sbi branch from 9384055 to b024a20 Compare May 29, 2020 20:19

pypa-bot removed the needs rebase or merge PR has conflicts with current master label May 29, 2020

uranusjr reviewed May 29, 2020

View reviewed changes

sbidoul added 6 commits May 31, 2020 18:33

Add REQUESTED support to install_wheel

21df86f

Mark top level requirements as REQUESTED

c9a4457

A constraint is not a top level requirement

a226f65

Rename is_direct to user_supplied

b9a19f6

Make sure user_supplied is propagated where needed

aa0c167

Uniform use of user supplied vocabulary

21bf4f5

sbidoul force-pushed the requested-sbi branch from b024a20 to 21bf4f5 Compare May 31, 2020 16:34

sbidoul mentioned this pull request Jun 24, 2020

Release resolver-centric beta of pip in early July #8206

Closed

pradyunsg merged commit fb68794 into pypa:master Jun 24, 2020

sbidoul deleted the requested-sbi branch June 24, 2020 13:20

atugushev mentioned this pull request Jun 28, 2020

Add pip>=20.2 support jazzband/pip-tools#1168

Merged

3 tasks

This was referenced Jul 28, 2020

Fails to run on pip==20.2b1 jazzband/pip-tools#1189

Closed

Possible regression in 20.2b1: AssertionError: a direct req shouldn't have a parent... #8637

Closed

uranusjr mentioned this pull request Apr 17, 2021

Feature request: keep track of how packages are installed (manually, automatically, ...) #9812

Closed

github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PEP 376 REQUESTED #8026

Implement PEP 376 REQUESTED #8026

sbidoul commented Apr 12, 2020 •

edited

Loading

sbidoul commented Apr 12, 2020

sbidoul commented Apr 12, 2020

uranusjr commented Apr 12, 2020 •

edited

Loading

sbidoul commented Apr 12, 2020

pradyunsg commented Apr 12, 2020

uranusjr commented Apr 13, 2020

BrownTruck commented May 5, 2020

pradyunsg May 15, 2020

sbidoul May 15, 2020

This comment has been minimized.

This comment has been minimized.

sbidoul commented May 15, 2020

This comment has been minimized.

sbidoul commented May 16, 2020

pfmoore left a comment

BrownTruck commented May 28, 2020

sbidoul commented May 29, 2020

uranusjr May 29, 2020

sbidoul May 30, 2020

uranusjr May 31, 2020

sbidoul May 31, 2020

pradyunsg commented Jun 24, 2020

Implement PEP 376 REQUESTED #8026

Implement PEP 376 REQUESTED #8026

Conversation

sbidoul commented Apr 12, 2020 • edited Loading

sbidoul commented Apr 12, 2020

sbidoul commented Apr 12, 2020

uranusjr commented Apr 12, 2020 • edited Loading

sbidoul commented Apr 12, 2020

pradyunsg commented Apr 12, 2020

uranusjr commented Apr 13, 2020

BrownTruck commented May 5, 2020

pradyunsg May 15, 2020

Choose a reason for hiding this comment

sbidoul May 15, 2020

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

sbidoul commented May 15, 2020

This comment has been minimized.

sbidoul commented May 16, 2020

pfmoore left a comment

Choose a reason for hiding this comment

BrownTruck commented May 28, 2020

sbidoul commented May 29, 2020

uranusjr May 29, 2020

Choose a reason for hiding this comment

sbidoul May 30, 2020

Choose a reason for hiding this comment

uranusjr May 31, 2020

Choose a reason for hiding this comment

sbidoul May 31, 2020

Choose a reason for hiding this comment

pradyunsg commented Jun 24, 2020

sbidoul commented Apr 12, 2020 •

edited

Loading

uranusjr commented Apr 12, 2020 •

edited

Loading