Re-install PEP 508 wheel dependencies when the version changes #6402

rouge8 · 2019-04-12T08:32:07Z

rouge8 · 2019-04-17T23:49:28Z

@cjerdonek can you take a look at this?

cjerdonek

I made some more superficial comments. Unfortunately, I don't feel qualified enough right now to evaluate the overall effect of the change and whether it's being implemented correctly, since this isn't something I've looked into before..

news/5780.bugfix

news/6402.bugfix

src/pip/_internal/req/constructors.py

tests/functional/test_install.py

tests/unit/test_req_install.py

src/pip/_internal/req/constructors.py

…q_string Previously, an InvalidRequirement would raise a NameError while trying to raise an InstallationError because `req` was not defined. Discovered while working on pypa#6402.

rouge8 · 2019-04-18T16:34:07Z

@cjerdonek thanks for the review 🙌 If you don't feel qualified to evaluate the overall effect, any suggestions for who is?

cam72cam · 2019-04-18T18:32:49Z

Just some anecdotal evidence I'll add: We've been using that patch since it was posted and it's been working well for months with daily deployments across dozens of servers.

cjerdonek

One comment.

cjerdonek · 2019-04-19T06:21:55Z

tests/functional/test_install.py

@@ -1391,6 +1391,50 @@ def test_install_pep508_with_url_in_install_requires(script):
    assert "Successfully installed packaging-15.3" in str(res), str(res)


+@pytest.mark.parametrize('create_dep, format', [
+    (create_test_package_with_setup, 'directory'),
+    (create_basic_wheel_for_package, 'wheel'),


Adding these strings "directory" and "wheel" and then doing if-checks in the test method to toggle behavior is a sign that this test would be better as two separate tests. To share code, you can put the common code in a private helper method used by both (e.g. still passing create_dep).

I went back and forth on that originally. I couldn't come up with a helper that makes sense, so I went with parametrization and the conditional.

Pushed up a version where it's split into two tests though

I couldn't come up with a helper that makes sense,

For the helper, you can basically use the body of the test function you had before more or less as is -- minus the if format toggles at the beginning and the end. Then each new test will have the form <beginning> <helper call> <end>, where the beginning and end portions are the if clauses that would have executed in your original test. There's a lot of repetition in what you have now in a long-ish test, so it would be helpful in understanding to be able to see the common structure being used.

This seems a lot harder to follow than the original parametrized version:

def function_that_does_a_lot(script, create_dep, dep_v1_path): dep_v2_path = create_dep(script, name='dep', version='2.0') pkga_path = create_basic_wheel_for_package( script, name='pkga', version='1.0', depends=['dep @ ' + path_to_url(dep_v1_path)], ) res = script.pip('install', pkga_path) assert "Successfully installed dep-1.0" in str(res), str(res) pkga_path.rmtree() # Updating the URL to the dependency installs the updated dependency pkga_path = create_basic_wheel_for_package( script, name='pkga', version='2.0', depends=['dep @ ' + path_to_url(dep_v2_path)], ) res = script.pip('install', pkga_path) assert "Successfully installed dep-2.0" in str(res), str(res) res = script.pip('install', pkga_path) return res def test_install_pep508_with_url_in_install_requires_url_change_wheel(script): dep_v1_path = create_basic_wheel_for_package( script, name='dep', version='1.0', ) res = function_that_does_a_lot(script, create_basic_wheel_for_package, dep_v1_path) # pip can determine the version from a wheel's filename, so the # dependency is not reinstalled if the URL doesn't change assert "Requirement already satisfied: dep==2.0" in str(res), str(res) def test_install_pep508_with_url_in_install_requires_url_change_directory( script): dep_v1_path = create_test_package_with_setup( script, name='dep', version='1.0', ) # Rename the package directory so it doesn't get overwritten when # creating the package for dep_v2 dep_v1_path.move(dep_v1_path.folder / 'dep_v1') dep_v1_path = dep_v1_path.folder / 'dep_v1' res = function_that_does_a_lot(script, create_test_package_with_setup, dep_v1_path) # pip can't determine versions from a directory name, so it will always # reinstall the dependency assert "Successfully installed dep-2.0" in str(res), str(res)

If you have ideas to improve this I'm open to suggestions because I don't see a version of this that's better than the parametrization.

Conceptually, it seems a lot simpler to me. Any way it's written, the reader will need to develop an understanding of what that large chunk of code is doing. How about starting by describing to me in words what the function is doing? (Not line by line but the goal in terms of setting up the test.)

Also, something else to consider is that if you make your create_dep function in the second case one that returns a path dependent on the version (i.e. doing a move in both cases), then it looks like you can pass only create_dep, which would move more code into the common function. Aside from the function definition in the second case, the body of both test functions would be just two lines.

…q_string (#6419) Previously, an InvalidRequirement would raise a NameError while trying to raise an InstallationError because `req` was not defined. Discovered while working on #6402.

pradyunsg

Haven't looked at most of the change/discussion here. Just noting a PEP compliance related question.

pradyunsg · 2019-04-21T19:26:30Z

src/pip/_internal/req/constructors.py

+        # Create an InstallRequirement for a PEP 508 URL with the same behavior
+        # as 'pip install req.url'
+        return install_req_from_line(
+            req.url, comes_from=comes_from, isolated=isolated,


What about extras?

PEP 508 allows them: url_req = name wsp* extras? wsp* urlspec wsp+ quoted_marker?

pip install requests[security] @ https://files.pythonhosted.org/packages/52/2c/514e4ac25da2b08ca5a464c50463682126385c4272c18193876e91f4bc38/requests-2.21.0.tar.gz

oooh good catch. I'll try figuring that out...

pip install requests[security] @ https://files.pythonhosted.org/packages/52/2c/514e4ac25da2b08ca5a464c50463682126385c4272c18193876e91f4bc38/requests-2.21.0.tar.gz

I think this actually doesn't work with pip install:

$ pip --version pip 19.0.3 from /Users/andy/.virtualenvs/tmp-de2709e9577734b/lib/python3.7/site-packages/pip (python 3.7) $ pip install requests[security]@https://files.pythonhosted.org/packages/52/2c/514e4ac25da2b08ca5a464c50463682126385c4272c18193876e91f4bc38/requests-2.21.0.tar.gz Requirement 'requests[security]@https://files.pythonhosted.org/packages/52/2c/514e4ac25da2b08ca5a464c50463682126385c4272c18193876e91f4bc38/requests-2.21.0.tar.gz' looks like a filename, but the file does not exist Processing ./requests[security]@https:/files.pythonhosted.org/packages/52/2c/514e4ac25da2b08ca5a464c50463682126385c4272c18193876e91f4bc38/requests-2.21.0.tar.gz Could not install packages due to an EnvironmentError: [Errno 2] No such file or directory: '/Users/andy/.virtualenvs/requests[security]@https:/files.pythonhosted.org/packages/52/2c/514e4ac25da2b08ca5a464c50463682126385c4272c18193876e91f4bc38/requests-2.21.0.tar.gz'

it works as expected in setup.py though.

It looks like there's lots of weird cases around PEP 508 URLs: #5788 (comment) 😕

I opened a pr : rouge8#1 to fix it

Thanks! I’ll try to update this PR with a test for the extras behavior.

@pradyunsg updated with a test for the extras behavior 🤞

cam72cam · 2019-05-14T16:37:09Z

What's required to get this PR in? I know that how PEP508 should be supported is not completely nailed down, but this would be a big QoL upgrade for those of us who need to use this feature today.

rouge8 · 2019-07-23T17:17:35Z

What's required to get this PR in? I know that how PEP508 should be supported is not completely nailed down, but this would be a big QoL upgrade for those of us who need to use this feature today.

I haven't had time to come back to it to deal with #6402 (comment) 😩

BrownTruck · 2019-07-27T08:00: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!

lmarvaud · 2019-10-10T13:32:35Z

@rouge8 alternatively to the PR (rouge8#1) here is the patch to apply after having merge master (if required and to save time here is the process I followed)

diff --git a/src/pip/_internal/req/constructors.py b/src/pip/_internal/req/constructors.py
index 95b7fb51..58f912c9 100644
--- a/src/pip/_internal/req/constructors.py
+++ b/src/pip/_internal/req/constructors.py
@@ -433,9 +433,17 @@ def install_req_from_req_string(
     if req.url:
         # Create an InstallRequirement for a PEP 508 URL with the same behavior
         # as 'pip install req.url'
-        return install_req_from_line(
-            req.url, comes_from=comes_from, isolated=isolated,
-            wheel_cache=wheel_cache, use_pep517=use_pep517
+        parts = parse_req_from_line(req.url, None)
+        constraint = False
+        options = None
+
+        return InstallRequirement(
+            parts.requirement, comes_from, link=parts.link, markers=parts.markers,
+            use_pep517=use_pep517, isolated=isolated,
+            options=options if options else {},
+            wheel_cache=wheel_cache,
+            constraint=constraint,
+            extras=req.extras,
         )
     return InstallRequirement(
         req, comes_from, isolated=isolated, wheel_cache=wheel_cache,

chrahunt

Two comments:

If we delegate to parse_req_from_line then we're opening ourselves up to supporting everything that it supports for linked dependencies, making it harder to make changes in the future. Instead can we make a more targeted change? It looks like the fundamental difference is here - for wheel URLs we extract the version and use that as the target for Requirement. We could do the same thing like
```
if req.url:
    link = Link(req.url)
    if link.is_wheel and not req.specifier:
        wheel = Wheel(link.filename)
        req.specifier = SpecifierSet("=={}".format(wheel.version))
```
This would give us a lot more flexibility going forward to adapt the implementation.
After the above, can we extract the parsing (not including the domains_not_allowed check) into a separate function that returns a RequirementParts? That will align with the rest of the functions in this module. I would construct the result like RequirementParts(req, None, None, set()) (instead of passing the link to RequirementParts) to reinforce that we're not doing any special post-processing on the link.

pradyunsg · 2020-02-06T07:12:19Z

This would give us a lot more flexibility going forward to adapt the implementation.

👍

@rouge8 could you update this PR to address the @chrahunt's review comments?

rouge8 · 2020-04-09T19:59:44Z

@pradyunsg it seems like this could be rewritten to take use #7612 and actually know when the URL changes, which makes #5780 and #7678 overlap right?

rouge8 · 2020-04-09T23:26:31Z

src/pip/_internal/req/constructors.py

+        # same behavior as 'pip install req.url'
+        parts = parse_req_from_line(req.url, None)
+        link = Link(req.url)
+        if link.is_wheel:


I dropped support for directories and non-wheel URLs in 2e082f9 because that was triggering an assertion error in the resolver. Hopefully this PR can be a stopgap until #7678 can be addressed in the new resolver?

rouge8 · 2020-05-15T02:44:55Z

@pradyunsg @chrahunt can you take another look?

uranusjr · 2020-05-15T07:49:35Z

Does the new resolver have the same issue? I kind of feel it’s the resolver’s job to detect a version change and ask for upgrade if needed. Although I’m not very worried with this change either—there is really no harm in parsing out the wheel link if we can.

With that said, the implementation of constructors looks a bit weird to me with this patch. What’s the difference between install_req_from_line and install_req_from_req_string? Why does the latter seem to parse a requirement twice? Why aren’t they just one function, or two functions but with one being a wrapper of the other (so we can add the PyPI packages depending on external URL check)?

rouge8 · 2020-05-15T23:10:49Z

Does the new resolver have the same issue? I kind of feel it’s the resolver’s job to detect a version change and ask for upgrade if needed. Although I’m not very worried with this change either—there is really no harm in parsing out the wheel link if we can.

It does, but then I ran into #8204 😩

With that said, the implementation of constructors looks a bit weird to me with this patch. What’s the difference between install_req_from_line and install_req_from_req_string? Why does the latter seem to parse a requirement twice? Why aren’t they just one function, or two functions but with one being a wrapper of the other (so we can add the PyPI packages depending on external URL check)?

I'm not really sure. I originally pulled this from #5780 (comment). Digging into the new resolver though, it looks like it's actually doing the same thing as this PR with make_requirement_from_install_req() which eventually calls install_req_from_line() inside of make_install_req_from_link().

pradyunsg · 2020-05-19T06:56:49Z

My understanding, right now, is that the new resolver works like this PR, so "bundling" this change as part of the new resolver, would be beneficial in terms of a rollout strategy for us here (since Hyrum's Law applies).

The ResolutionTooDeep issue would be fixed as we move further, on the new resolver's implementation.

rouge8 · 2020-05-19T07:47:47Z

so "bundling" this change as part of the new resolver, would be beneficial in terms of a rollout strategy for us here

As in only fixing it in the new resolver going forward?

In that case let me know if there’s any logging or debugging I can do to help with the ResolutionTooDeep issue. The actual fix is probably over my head, but I would love to help in any way to get my teams at work off of pip 18.1 and right now the PEP 508 URLs are the blocker for us.

pradyunsg · 2020-05-20T15:04:02Z

@rouge8 I've gone ahead and made a relevant comment on #8204, specifically we started moving the needle in terms of the ResolutionTooDeep issue. Feedback (positive and negative!) is welcome there! :)

rouge8 force-pushed the gh-5780 branch from a139ae8 to d37d0eb Compare April 12, 2019 08:56

rouge8 mentioned this pull request Apr 12, 2019

Updating remote links with new URLs for PEP508 functionallity #5780

Open

rouge8 changed the title ~~Re-install PEP 508 dependencies when the URL changes~~ Fix #5780: Re-install PEP 508 dependencies when the URL changes Apr 17, 2019

cjerdonek reviewed Apr 18, 2019

View reviewed changes

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

rouge8 force-pushed the gh-5780 branch from a6af304 to af8b6af Compare April 18, 2019 09:19

rouge8 commented Apr 18, 2019

View reviewed changes

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

rouge8 mentioned this pull request Apr 18, 2019

Fix NameError when handling InvalidRequirement in install_req_from_req_string #6419

Merged

rouge8 force-pushed the gh-5780 branch from 2003ed1 to e2bbd12 Compare April 18, 2019 16:25

cjerdonek reviewed Apr 19, 2019

View reviewed changes

pradyunsg reviewed Apr 21, 2019

View reviewed changes

BrownTruck added the needs rebase or merge PR has conflicts with current master label Jul 27, 2019

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Oct 10, 2019

rouge8 requested a review from pradyunsg October 10, 2019 18:23

chrahunt suggested changes Dec 8, 2019

View reviewed changes

chrahunt changed the title ~~Fix #5780: Re-install PEP 508 dependencies when the URL changes~~ Re-install PEP 508 wheel dependencies when the URL changes Dec 9, 2019

chrahunt changed the title ~~Re-install PEP 508 wheel dependencies when the URL changes~~ Re-install PEP 508 wheel dependencies when the version changes Dec 9, 2019

pradyunsg added pradyunsg-moving-issues and removed pradyunsg-moving-issues labels Jan 14, 2020

sbidoul mentioned this pull request Feb 6, 2020

pip does not reinstall an already installed package from a different direct url requirement #7678

Closed

rouge8 and others added 11 commits April 9, 2020 11:19

Fixes for existing tests

d938a8e

Fix news fragments

51d6ab7

Rework tests to use local wheels not require network access

a63ca36

Pass use_pep517 along

f7cdf3b

PR feedback and tests

2116c04

Split the test into two tests

251a002

Allow extra in re-install PEP 508 dependencies when the URL changes

caf1d65

Lint

a4a121a

Fix existing tests

e3e359c

Add test for extras

3a5f27a

Rebase and remove bad options

c348730

rouge8 force-pushed the gh-5780 branch from 4cceb48 to c348730 Compare April 9, 2020 18:28

Test fixes

31c1f4f

Limit support for directories as PEP508 dependencies

2e082f9

rouge8 force-pushed the gh-5780 branch from f4eb3c8 to 2e082f9 Compare April 9, 2020 23:05

mypy fix

e5bcc32

rouge8 commented Apr 9, 2020

View reviewed changes

rouge8 requested review from pradyunsg and chrahunt April 9, 2020 23:26

pradyunsg added C: new resolver and removed C: new resolver labels May 19, 2020

rouge8 closed this Aug 4, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-install PEP 508 wheel dependencies when the version changes #6402

Re-install PEP 508 wheel dependencies when the version changes #6402

rouge8 commented Apr 12, 2019

rouge8 commented Apr 17, 2019

cjerdonek left a comment

rouge8 commented Apr 18, 2019

cam72cam commented Apr 18, 2019

cjerdonek left a comment

cjerdonek Apr 19, 2019

rouge8 Apr 19, 2019

cjerdonek Apr 19, 2019

rouge8 Apr 19, 2019

cjerdonek Apr 21, 2019

cjerdonek Apr 21, 2019

pradyunsg left a comment

pradyunsg Apr 21, 2019

rouge8 Apr 22, 2019

rouge8 Apr 22, 2019

lmarvaud Oct 10, 2019

rouge8 Oct 10, 2019

rouge8 Oct 10, 2019

cam72cam commented May 14, 2019

rouge8 commented Jul 23, 2019

BrownTruck commented Jul 27, 2019

lmarvaud commented Oct 10, 2019

chrahunt left a comment •

edited

Loading

pradyunsg commented Feb 6, 2020

rouge8 commented Apr 9, 2020

rouge8 Apr 9, 2020

rouge8 commented May 15, 2020

uranusjr commented May 15, 2020

rouge8 commented May 15, 2020

pradyunsg commented May 19, 2020 •

edited

Loading

rouge8 commented May 19, 2020

pradyunsg commented May 20, 2020

Re-install PEP 508 wheel dependencies when the version changes #6402

Re-install PEP 508 wheel dependencies when the version changes #6402

Conversation

rouge8 commented Apr 12, 2019

rouge8 commented Apr 17, 2019

cjerdonek left a comment

Choose a reason for hiding this comment

rouge8 commented Apr 18, 2019

cam72cam commented Apr 18, 2019

cjerdonek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pradyunsg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cam72cam commented May 14, 2019

rouge8 commented Jul 23, 2019

BrownTruck commented Jul 27, 2019

lmarvaud commented Oct 10, 2019

chrahunt left a comment • edited Loading

Choose a reason for hiding this comment

pradyunsg commented Feb 6, 2020

rouge8 commented Apr 9, 2020

Choose a reason for hiding this comment

rouge8 commented May 15, 2020

uranusjr commented May 15, 2020

rouge8 commented May 15, 2020

pradyunsg commented May 19, 2020 • edited Loading

rouge8 commented May 19, 2020

pradyunsg commented May 20, 2020

chrahunt left a comment •

edited

Loading

pradyunsg commented May 19, 2020 •

edited

Loading