Ensure `pip-tools` unions dependencies of multiple declarations of a package with different extras #1486

richafrank · 2021-09-20T11:57:29Z

Fix for #1451 that uses the same simplification in #1452 but handles the case of combining editables (that lack names) as well, e.g. test_editable_package_constraint_without_non_editable_duplicate.

This reverts the original solution for the test case above and replaces it. The original solution was to prepare the nameless ireqs early, so that they have a name. However:

    Preparing the ireq is side-effect-ful and can only be done once for an
    instance, so we use a proxy instead. combine_install_requirements may
    use the given ireq as a template for its aggregate result, mutating it
    further by combining extras, etc. In that situation, we don't want that
    aggregate ireq to be prepared prior to mutation, since its dependencies
    will be frozen with those of only a subset of extras.

    i.e. We both want its name early (via preparation), but we also need to
    prepare it after any mutation for combination purposes. So we use a
    proxy here for the early preparation.

This avoids issues with the ireq instances and the cache needing updates after mutating the extras for the combined_ireq, specifically #1451 . It means an extra preparation step for ireqs lacking names (preparing the proxy in addition to the ireq itself eventually), but that seems worthwhile (in my opinion).

Contributor checklist

Provided the tests for the changes.
Assure PR title is short, clear, and good to be included in the user-oriented changelog

Maintainer checklist

Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

AndydeCleyre · 2021-09-21T21:43:25Z

I'm not sure if it would be expected, but I notice that this does not resolve #852, which is reproducible as (courtesy of @atugushev):

$ printf '%s\n' '-e git+https://github.com/edx/django-rest-framework-oauth.git@0a43e8525f1e3048efe4bc70c03de308a277197c#egg=djangorestframework-oauth==1.1.1' 'edx-enterprise==1.7.2' >requirements.in
$ seq 1 10 | while read i; do echo "---- $i ----" && pip-compile -rqo- requirements.in | grep '^edx-opaque-keys'; done
---- 1 ----
edx-opaque-keys[django]==1.0.1
---- 2 ----
edx-opaque-keys==1.0.1
---- 3 ----
edx-opaque-keys==1.0.1
---- 4 ----
edx-opaque-keys[django]==1.0.1
---- 5 ----
edx-opaque-keys==1.0.1
---- 6 ----
edx-opaque-keys[django]==1.0.1
---- 7 ----
edx-opaque-keys[django]==1.0.1
---- 8 ----
edx-opaque-keys==1.0.1
---- 9 ----
edx-opaque-keys==1.0.1
---- 10 ----
edx-opaque-keys==1.0.1

richafrank · 2021-09-22T22:28:45Z

I'm not sure if it would be expected, but I notice that this does not resolve #852

Ah, interesting! Looks like related code, but a separate issue. I think I have a fix, but it might take a little time to formulate a good unit test (that doesn't require all of django). I can work on another PR for that.

richafrank · 2021-09-25T18:16:43Z

@AndydeCleyre Since this is still open, I pushed the fix for #852 here.

AndydeCleyre · 2021-09-25T18:39:41Z

Thank you, this is greatly appreciated!

AndydeCleyre · 2021-10-01T18:09:46Z

Can you say whether it would, or why it would not, make sense to integrate the new prep-proxy-to-get-name-to-get-key behavior in our utils.key_from_ireq function?

richafrank · 2021-10-03T19:41:08Z

Can you say whether it would, or why it would not, make sense to integrate the new prep-proxy-to-get-name-to-get-key behavior in our utils.key_from_ireq function?

Great question. IMO based on the general intent of key_from_ireq, I think it probably does make sense to integrate this behavior there. I imagine some bugs (reported and otherwise) would get squashed. A few considerations though:

Preparing the proxy requires a piptools.repositories.base.BaseRepository instance and means a new dependency for everything that calls key_from_ireq.
key_from_ireq is very low level (as per its location in utils), and as I look at updating it to accept a BaseRepository argument, many, many call sites (across many modules) need updating. This isn't a tiny change, so while this change may still prove beneficial, I wouldn't recommend including it as part of this bug fix, if the two can be delivered incrementally (which I think they can).
I think I have most of the compile tests passing with this change, but the sync code paths don't already offer a BaseRepository instance to pass around, so we might need to discuss the best approach there.
Because of the breadth of the key_from_ireq call sites, all these changes should definitely get many eyes on them. I'm certainly not an expert on all of them.
We would need to assess whether each call site requires this new behavior and dependency, but also whether there are any speed concerns that require caching, like I included in the one call site I have here, and what the lifetime of that cache and its elements are. (That's a great question for reviewers of this PR too.)

Let me know what you think.

AndydeCleyre · 2021-10-03T20:57:38Z

@richafrank Thanks, you've articulated everything perfectly. I'm approving this now.

@atugushev When you can please have a look at this and the above comment.

AndydeCleyre

This is looking great and good to go as far as I'm concerned, with the note that we should figure out what to do about key_from_ireq as per the discussion, as a follow up task. This is an important fix, thank you!

atugushev

@richafrank, awesome fix! 🎉

atugushev · 2021-10-03T21:19:19Z

tests/test_resolver.py

@@ -299,6 +300,43 @@ def test_combine_install_requirements(repository, from_line):
    assert str(combined_all.req.specifier) == "<3.2,==3.1.1,>3.0"


+def test_combine_install_requirements_extras(repository, from_line, make_package):


Could you split each test block into different tests or parametrize tests if possible?

Sounds good. I split it into two tests.

atugushev · 2021-10-03T22:14:19Z

When you can please have a look at this and the above comment.

@AndydeCleyre Nice thoughts about key_from_ireq improvement. Let's discuss it in a separate issue 👍🏻

atugushev · 2021-10-03T22:17:23Z

@richafrank I've updated the PR's description to link fixed issues. Could you check please if everything is correct?

with different extras resulted in a non-deterministic choice between the resolved dependencies of the different declared sets of extras.

Pointed out in jazzband#1451 (comment)

Fixes jazzband#852

richafrank · 2021-10-04T01:34:13Z

I've updated the PR's description to link fixed issues. Could you check please if everything is correct?

Yep, looks good to me.

richafrank force-pushed the combine-extras-deps branch from c30cd2b to a04f0fe Compare September 20, 2021 13:37

richafrank mentioned this pull request Sep 20, 2021

Pip-compile is non-deterministic with multiple declarations of a dependency specifying extras #1451

Closed

AndydeCleyre requested a review from atugushev September 20, 2021 20:46

AndydeCleyre mentioned this pull request Sep 20, 2021

Partially revert 5.1.2 change that broke extras (seeks to address #1451) #1452

Closed

4 tasks

richafrank force-pushed the combine-extras-deps branch from a04f0fe to a23ec03 Compare September 21, 2021 11:27

AndydeCleyre requested a review from jdufresne September 21, 2021 17:31

richafrank force-pushed the combine-extras-deps branch from a23ec03 to 4cf1198 Compare September 22, 2021 22:28

AndydeCleyre mentioned this pull request Sep 25, 2021

Updating requirements with extra does not update related package versions #768

Closed

richafrank force-pushed the combine-extras-deps branch from 3322017 to d49534d Compare October 3, 2021 01:25

AndydeCleyre approved these changes Oct 3, 2021

View reviewed changes

atugushev changed the title ~~Ensure pip-tools unions dependencies of multiple declarations of a package with different extras~~ Ensure pip-tools unions dependencies of multiple declarations of a package with different extras Oct 3, 2021

atugushev reviewed Oct 3, 2021

View reviewed changes

atugushev added the bug fix label Oct 3, 2021

atugushev added this to the 6.3.1 milestone Oct 3, 2021

richafrank added 4 commits October 3, 2021 21:32

Fix a regression where multiple declarations of a dependency

d720fe6

with different extras resulted in a non-deterministic choice between the resolved dependencies of the different declared sets of extras.

InstallRequirement.extras is expected to be a set anyway

a69540d

Pointed out in jazzband#1451 (comment)

Ensure extras are propagated when ireq has a req

1942484

Fixes jazzband#852

Move test blocks into their own test cases

ddb7c3a

richafrank force-pushed the combine-extras-deps branch from ac2431c to ddb7c3a Compare October 4, 2021 01:34

Merge branch 'master' into combine-extras-deps

9f3d5b7

atugushev approved these changes Oct 7, 2021

View reviewed changes

atugushev merged commit fef68a9 into jazzband:master Oct 8, 2021

richafrank deleted the combine-extras-deps branch October 8, 2021 22:33

AndydeCleyre mentioned this pull request Oct 13, 2021

Prevent loss of deep dependencies of local requirements #1506

Closed

6 tasks

atugushev added the resolver Related to dependency resolver label Oct 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure `pip-tools` unions dependencies of multiple declarations of a package with different extras #1486

Ensure `pip-tools` unions dependencies of multiple declarations of a package with different extras #1486

richafrank commented Sep 20, 2021 •

edited by atugushev

Loading

AndydeCleyre commented Sep 21, 2021

richafrank commented Sep 22, 2021

richafrank commented Sep 25, 2021

AndydeCleyre commented Sep 25, 2021

AndydeCleyre commented Oct 1, 2021

richafrank commented Oct 3, 2021

AndydeCleyre commented Oct 3, 2021

AndydeCleyre left a comment

atugushev left a comment

atugushev Oct 3, 2021

richafrank Oct 4, 2021

atugushev commented Oct 3, 2021

atugushev commented Oct 3, 2021

richafrank commented Oct 4, 2021

		@@ -299,6 +300,43 @@ def test_combine_install_requirements(repository, from_line):
		assert str(combined_all.req.specifier) == "<3.2,==3.1.1,>3.0"


		def test_combine_install_requirements_extras(repository, from_line, make_package):

Ensure pip-tools unions dependencies of multiple declarations of a package with different extras #1486

Ensure pip-tools unions dependencies of multiple declarations of a package with different extras #1486

Conversation

richafrank commented Sep 20, 2021 • edited by atugushev Loading

Contributor checklist

Maintainer checklist

AndydeCleyre commented Sep 21, 2021

richafrank commented Sep 22, 2021

richafrank commented Sep 25, 2021

AndydeCleyre commented Sep 25, 2021

AndydeCleyre commented Oct 1, 2021

richafrank commented Oct 3, 2021

AndydeCleyre commented Oct 3, 2021

AndydeCleyre left a comment

Choose a reason for hiding this comment

atugushev left a comment

Choose a reason for hiding this comment

atugushev Oct 3, 2021

Choose a reason for hiding this comment

richafrank Oct 4, 2021

Choose a reason for hiding this comment

atugushev commented Oct 3, 2021

atugushev commented Oct 3, 2021

richafrank commented Oct 4, 2021

Ensure `pip-tools` unions dependencies of multiple declarations of a package with different extras #1486

Ensure `pip-tools` unions dependencies of multiple declarations of a package with different extras #1486

richafrank commented Sep 20, 2021 •

edited by atugushev

Loading