Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Present found conflicts when discarding some criterion #10937

Merged

Conversation

astrojuanlu
Copy link
Contributor

@astrojuanlu astrojuanlu commented Mar 3, 2022

Fixes gh-9254.
Closes gh-10258.
See #10258 (comment) for inspiration.

This is a proof of concept of an alternative approach to gh-10258, hoping that it will (1) provide a better UX when "backtracking" (in quotes, see below) happens, and (2) do it in a way that is acceptable for the pip maintainers. It doesn't have tests or a news entry yet, hoping that I can get maintainer approval & community consensus first.

First of all, I did all of this in a hurry, so forgive me if I'm overloading the terms "requirement", "candidate", "round", "resolve", and such. I only spent 3 hours looking at the pip codebase, and I didn't find high-level docs on "how the new resolver works". By the way, the code comments are great 💯

The tricky thing was summarized, if I understood correctly, in this comment by @pradyunsg:

I'm also pretty sure I wrote that code, and arguably, doing what I'm suggesting here would have been a better match for what people consider "backtracking" than what is implemented there. 😅

I don't think using resolvelib's "round" as a unit of backtracking is particularly useful for deciding when to present conflicts -- we can have conflicts that cause a package to be rejected within a single "round" as well, and I think presenting those cases is much more useful/informative.

Indeed, what users call "backtracking" might be two things:

  • Actual backtracking messages, as defined in the codebase: INFO: pip is looking at multiple versions of pyjwt to determine which version is compatible with other requirements. This could take a while..
  • Retrial of older and older versions of a given requirement while pip is trying to satisfy some constraints, which comes before the backtracking phase, but it looks like "backtracking".

In this implementation I'm trying to hook into "the right place" so that conflict information is presented immediately when discarding a certain candidate.

Example of output with direct, impossible resolution example from the documentation:

$ python -m pip install "pytest < 4.6" "pytest-cov==2.12.1" --no-cache-dir
Collecting pytest<4.6
  Downloading pytest-4.5.0-py2.py3-none-any.whl (227 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 227.5/227.5 KB 4.8 MB/s eta 0:00:00
Collecting pytest-cov==2.12.1
  Downloading pytest_cov-2.12.1-py2.py3-none-any.whl (20 kB)
Will try a different candidate, due to conflict:
    The user requested pytest<4.6
    pytest-cov 2.12.1 depends on pytest>=4.6
ERROR: Cannot install pytest-cov==2.12.1 and pytest<4.6 because these package versions have conflicting dependencies.

The conflict is caused by:
    The user requested pytest<4.6
    pytest-cov 2.12.1 depends on pytest>=4.6

To fix this you could try to:
1. loosen the range of package versions you've specified
2. remove package versions to allow pip attempt to solve the dependency conflict

ERROR: ResolutionImpossible: for help visit https://pip.pypa.io/en/latest/topics/dependency-resolution/#dealing-with-dependency-conflicts

Example of so-called "backtracking" but-not-quite, when pip is reaching out to older versions of an unpinned dependency to try to satisfy the constraints (and which doesn't produce a "this will take a while" message):

$ pip install "botocore~=1.10.0" boto3 --no-cache-dir
Collecting botocore~=1.10.0
  Downloading botocore-1.10.84-py2.py3-none-any.whl (4.5 MB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 4.5/4.5 MB 6.6 MB/s eta 0:00:00
Collecting boto3
  Downloading boto3-1.21.11-py3-none-any.whl (132 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 132.2/132.2 KB 6.9 MB/s eta 0:00:00
Collecting jmespath<1.0.0,>=0.7.1
  Downloading jmespath-0.10.0-py2.py3-none-any.whl (24 kB)
Collecting docutils>=0.10
  Downloading docutils-0.18.1-py2.py3-none-any.whl (570 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 570.0/570.0 KB 7.9 MB/s eta 0:00:00
Collecting python-dateutil<3.0.0,>=2.1
  Downloading python_dateutil-2.8.2-py2.py3-none-any.whl (247 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 247.7/247.7 KB 7.8 MB/s eta 0:00:00
Collecting s3transfer<0.6.0,>=0.5.0
  Downloading s3transfer-0.5.2-py3-none-any.whl (79 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 79.5/79.5 KB 12.9 MB/s eta 0:00:00
Will try a different candidate, due to conflict:
    The user requested botocore~=1.10.0
    boto3 1.21.11 depends on botocore<1.25.0 and >=1.24.11
Collecting boto3
  Downloading boto3-1.21.10-py3-none-any.whl (132 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 132.2/132.2 KB 7.8 MB/s eta 0:00:00
Will try a different candidate, due to conflict:
    The user requested botocore~=1.10.0
    boto3 1.21.10 depends on botocore<1.25.0 and >=1.24.10
  Downloading boto3-1.21.9-py3-none-any.whl (132 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 132.2/132.2 KB 7.5 MB/s eta 0:00:00
Will try a different candidate, due to conflict:
    The user requested botocore~=1.10.0
    boto3 1.21.9 depends on botocore<1.25.0 and >=1.24.9
  Downloading boto3-1.21.8-py3-none-any.whl (132 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 132.2/132.2 KB 8.7 MB/s eta 0:00:00
Will try a different candidate, due to conflict:
    The user requested botocore~=1.10.0
    boto3 1.21.8 depends on botocore<1.25.0 and >=1.24.8
  Downloading boto3-1.21.7-py3-none-any.whl (132 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 132.2/132.2 KB 9.2 MB/s eta 0:00:00
Will try a different candidate, due to conflict:
    The user requested botocore~=1.10.0
    boto3 1.21.7 depends on botocore<1.25.0 and >=1.24.7
  Downloading boto3-1.21.6-py3-none-any.whl (132 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 132.2/132.2 KB 9.9 MB/s eta 0:00:00
Will try a different candidate, due to conflict:
    The user requested botocore~=1.10.0
    boto3 1.21.6 depends on botocore<1.25.0 and >=1.24.6
  Downloading boto3-1.21.5-py3-none-any.whl (132 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 132.1/132.1 KB 9.7 MB/s eta 0:00:00
Will try a different candidate, due to conflict:
    The user requested botocore~=1.10.0
    boto3 1.21.5 depends on botocore<1.25.0 and >=1.24.5
  Downloading boto3-1.21.4-py3-none-any.whl (132 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 132.1/132.1 KB 7.2 MB/s eta 0:00:00
^CERROR: Operation cancelled by user

Example with super long backtracking as officially defined in the pip codebase (full output, too long for GitHub):

root@f4372a0219b2:/opt/airflow# pip install ".[devel_all]" --upgrade --upgrade-strategy eager "dill<0.3.3" "certifi<2021.0.0" "google-ads<14.0.1"
Processing /opt/airflow
  Preparing metadata (setup.py) ... done
...
Requirement already satisfied: ipython-genutils in /usr/local/lib/python3.7/site-packages (from nbformat>=5.1.2->papermill[all]>=1.2.1->apache-airflow==2.3.0.dev0) (0.2.0)
Requirement already satisfied: jupyter-core in /usr/local/lib/python3.7/site-packages (from nbformat>=5.1.2->papermill[all]>=1.2.1->apache-airflow==2.3.0.dev0) (4.9.2)
Requirement already satisfied: locket in /usr/local/lib/python3.7/site-packages (from partd>=0.3.10->dask<2021.6.1,>=2.9.0->apache-airflow==2.3.0.dev0) (0.2.1)
Will try a different candidate, due to conflict:
    flask-appbuilder 3.4.4 depends on PyJWT<2.0.0 and >=1.7.1
    snowflake-connector-python 2.7.1 depends on pyjwt<3.0.0
    yandexcloud 0.146.0 depends on pyjwt>=1.7.1
    pygithub 1.54.1 depends on pyjwt<2.0
    adal 1.2.7 depends on PyJWT<3 and >=1.0.0
    flask-jwt-extended 3.25.1 depends on PyJWT<2.0 and >=1.6.4
    pyjwt[crypto] 2.3.0 depends on pyjwt 2.3.0 (from https://files.pythonhosted.org/packages/2a/4d/67cc66a0c49003dc216fc73db2d05a3b80c7193167fd113da1f2c678ac2a/PyJWT-2.3.0-py3-none-any.whl#sha256=e0c4bb8d9f0af0c7f5b1ec4c5036309617d03d56932877f2f7a0beeb5318322f (from https://pypi.org/simple/pyjwt/) (requires-python:>=3.6))
INFO: pip is looking at multiple versions of pyjwt to determine which version is compatible with other requirements. This could take a while.
Collecting PyJWT<2.0.0,>=1.7.1
  Downloading PyJWT-1.7.1-py2.py3-none-any.whl (18 kB)
Will try a different candidate, due to conflict:
    flask-appbuilder 3.4.4 depends on PyJWT<2.0.0 and >=1.7.1
    snowflake-connector-python 2.7.1 depends on pyjwt<3.0.0
    yandexcloud 0.146.0 depends on pyjwt>=1.7.1
    pygithub 1.54.1 depends on pyjwt<2.0
    adal 1.2.7 depends on PyJWT<3 and >=1.0.0
    flask-jwt-extended 3.25.1 depends on PyJWT<2.0 and >=1.6.4
    pyjwt[crypto] 2.3.0 depends on pyjwt 2.3.0 (from https://files.pythonhosted.org/packages/2a/4d/67cc66a0c49003dc216fc73db2d05a3b80c7193167fd113da1f2c678ac2a/PyJWT-2.3.0-py3-none-any.whl#sha256=e0c4bb8d9f0af0c7f5b1ec4c5036309617d03d56932877f2f7a0beeb5318322f (from https://pypi.org/simple/pyjwt/) (requires-python:>=3.6))
INFO: pip is looking at multiple versions of pyflakes to determine which version is compatible with other requirements. This could take a while.
Collecting pyflakes<2.4.0,>=2.3.0
  Downloading pyflakes-2.3.1-py2.py3-none-any.whl (68 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 68.8/68.8 KB 6.0 MB/s eta 0:00:00
Will try a different candidate, due to conflict:
    flask-appbuilder 3.4.4 depends on PyJWT<2.0.0 and >=1.7.1
    snowflake-connector-python 2.7.1 depends on pyjwt<3.0.0
    yandexcloud 0.146.0 depends on pyjwt>=1.7.1
    pygithub 1.54.1 depends on pyjwt<2.0
    adal 1.2.7 depends on PyJWT<3 and >=1.0.0
    flask-jwt-extended 3.25.1 depends on PyJWT<2.0 and >=1.6.4
    pyjwt[crypto] 2.3.0 depends on pyjwt 2.3.0 (from https://files.pythonhosted.org/packages/2a/4d/67cc66a0c49003dc216fc73db2d05a3b80c7193167fd113da1f2c678ac2a/PyJWT-2.3.0-py3-none-any.whl#sha256=e0c4bb8d9f0af0c7f5b1ec4c5036309617d03d56932877f2f7a0beeb5318322f (from https://pypi.org/simple/pyjwt/) (requires-python:>=3.6))
...

@pfmoore
Copy link
Member

pfmoore commented Mar 3, 2022

Thanks for doing this. Note that while this is fine as a proof of concept, ultimately the changes to the vendored resolvelib will need to be factored out and submitted as a PR to resolvelib itself (I don't see this as an issue, just noting it for the record in case you weren't aware).

@astrojuanlu
Copy link
Contributor Author

Thanks @pfmoore! Yep, the changes spanned both pip._internal and pip._vendor so I assume that, if this gets "approved", it won't be merged as-is, but rather I'll need to send a PR to https://github.com/sarugaku/resolvelib/ instead.

@pradyunsg
Copy link
Member

Thanks for doing this! This works for me in principle.

I'd repurpose the backtracking hook to be called at this point in the resolver (it was added for precisely this usecase, so it'd be better to call it at a more sensible spot). That will also fix the presentation of the "this might take a while" message that we have, and fix more properly in the model that we have here.

Beyond that, I think we probably want to decide how often the warnings are presented and what their verbosity + presentation style should be. Those are minor nitpicky items that we shouldn't spend too much time on (bikeshedding, law of triviality blah blah) so... let's get a resolvelib-side fix in for this! :)

@astrojuanlu
Copy link
Contributor Author

Thanks @pradyunsg! I just pushed a new commit, repurposing the backtracking hook to present the criterion information. I'm not 100 % sure if you were suggesting getting rid of the "this will take a while" messages that were dependent on the number of rounds on the package, but I did it - I can partially roll it back. Let me know if this is what you had in mind and I'll send the PR to resolvelib.

Beyond that, I think we probably want to decide how often the warnings are presented and what their verbosity + presentation style should be. Those are minor nitpicky items that we shouldn't spend too much time on (bikeshedding, law of triviality blah blah) so... let's get a resolvelib-side fix in for this! :)

Sure thing. I don't have a strong opinion - the current output looks fine for me but I acknowledge that folks might want something better than just "fine" :)

This is how it looks for the airflow situation:

Collecting elasticsearch>7
  Downloading elasticsearch-7.17.1-py2.py3-none-any.whl (385 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 385.8/385.8 KB 6.7 MB/s eta 0:00:00
Requirement already satisfied: curlify>=2.1.0 in /usr/local/lib/python3.7/site-packages (from facebook-business>=6.0.2->apache-airflow==2.3.0.dev0) (2.2.1)
Requirement already satisfied: aiohttp in /usr/local/lib/python3.7/site-packages (from facebook-business>=6.0.2->apache-airflow==2.3.0.dev0) (3.8.1)
Requirement already satisfied: pycountry>=19.8.18 in /usr/local/lib/python3.7/site-packages (from facebook-business>=6.0.2->apache-airflow==2.3.0.dev0) (22.1.10)
INFO: Will try a different candidate, due to conflict:
    apache-airflow[devel-all] 2.3.0.dev0 depends on importlib_metadata>=1.7
    apache-airflow[devel-all] 2.3.0.dev0 depends on importlib-metadata>=4.4
    nox 2020.12.31 depends on importlib-metadata; python_version < "3.8"
    alembic 1.7.6 depends on importlib-metadata; python_version < "3.9"
    argcomplete 1.12.3 depends on importlib-metadata<5 and >=0.23; python_version == "3.7"
    click 8.0.4 depends on importlib-metadata; python_version < "3.8"
    flake8 4.0.1 depends on importlib-metadata<4.3; python_version < "3.8"
Collecting flake8>=3.6.0
  Downloading flake8-4.0.0-py2.py3-none-any.whl (64 kB)
     ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 64.1/64.1 KB 14.5 MB/s eta 0:00:00
INFO: Will try a different candidate, due to conflict:
    apache-airflow[devel-all] 2.3.0.dev0 depends on importlib_metadata>=1.7
    apache-airflow[devel-all] 2.3.0.dev0 depends on importlib-metadata>=4.4
    nox 2020.12.31 depends on importlib-metadata; python_version < "3.8"
    alembic 1.7.6 depends on importlib-metadata; python_version < "3.9"
    argcomplete 1.12.3 depends on importlib-metadata<5 and >=0.23; python_version == "3.7"
    click 8.0.4 depends on importlib-metadata; python_version < "3.8"
    flake8 4.0.0 depends on importlib-metadata<4.3; python_version < "3.8"

@pradyunsg
Copy link
Member

I didn't imply that we should remove the existing messages that this might take a while; and, yes, it does look like we might want to think about how "noisy" this might end up being.

Regardless, I think the obvious next step here is to be make a PR on the resolvelib side; so... Let's do that and then come back to the pip side of the story?

astrojuanlu added a commit to astrojuanlu/resolvelib that referenced this pull request Mar 4, 2022
@astrojuanlu
Copy link
Contributor Author

I didn't imply that we should remove the existing messages that this might take a while; and, yes, it does look like we might want to think about how "noisy" this might end up being.

Sounds good to me

Regardless, I think the obvious next step here is to be make a PR on the resolvelib side; so... Let's do that and then come back to the pip side of the story?

Done: sarugaku/resolvelib#101

astrojuanlu added a commit to astrojuanlu/resolvelib that referenced this pull request Mar 4, 2022
@uranusjr
Copy link
Member

How do we progress this now the resolvelib changes are merged?

@astrojuanlu
Copy link
Contributor Author

Good question - I think @pradyunsg has some concerns about the verbosity of the output, or at least wanted to discuss a bit. On my side, I think the only missing bit is bringing a new vendorized version of resolvelib after sarugaku/resolvelib#101 was merged.

@pradyunsg
Copy link
Member

pradyunsg commented May 4, 2022

You can bring the in-development version of resolvelib in by changing vendor.txt to have resolvelib @ git+https://... and running nox -s vendoring after that -- we won't merge the PR with that but it'll still be a useful thing, since it'll let you test things without waiting on a resolvelib reelase.

And if you wanna pair program on this sometime (in a UK evening): https://calendly.com/pradyunsg/open-calendar :)

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Feb 7, 2023
@astrojuanlu
Copy link
Contributor Author

Just remembered about this today because of a comment in another issue. Should I try to push it over the line?

@pradyunsg
Copy link
Member

That would be welcome. I'm still unsure about how to handle the verbosity problem here and don't have any bright ideas about it either. :/

@astrojuanlu
Copy link
Contributor Author

FTR, resolvelib 0.9.0 includes sarugaku/resolvelib#101

@astrojuanlu
Copy link
Contributor Author

astrojuanlu commented Feb 27, 2023

@pradyunsg I think this was superseded by #11783 :)

(Not quite)

@astrojuanlu astrojuanlu force-pushed the present-found-conflicts-when-discarding branch from c2a4343 to 7c82696 Compare February 27, 2023 09:35
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Feb 27, 2023
@astrojuanlu
Copy link
Contributor Author

Okay, with the upstreamed changes from resolvelib, the final diff is very small. I changed to debug log level so it won't be seen by default, and maybe folks finding backtracking problems can call pip with -vvvvvvvvv. Thoughts?

@pradyunsg
Copy link
Member

This PR still needs a news entry (the details link on the failure for the context + explanation) and it's still marked as a draft. :)

Other than that, a unit test would be nice to have for this!

@pradyunsg
Copy link
Member

maybe folks finding backtracking problems can call pip with -vvvvvvvvv. Thoughts?

It'd be just -vv and, yea, that works for me.

@astrojuanlu
Copy link
Contributor Author

Yeah, -vvvvvvvvvvvvvvvvv was an exaggeration 😄

Sure thing, I'll add a news entry and try putting a test too

@pradyunsg
Copy link
Member

Yup yup, I trust you know that but I don't think an arbitrary reader will, so wanted to explicitly flag that! :)

@pradyunsg
Copy link
Member

@astrojuanlu Could you add a news file entry to this PR and mark it ready for review?

@astrojuanlu astrojuanlu force-pushed the present-found-conflicts-when-discarding branch from 7c82696 to 0ee6cf9 Compare March 26, 2023 12:27
@astrojuanlu astrojuanlu marked this pull request as ready for review March 26, 2023 12:27
@astrojuanlu
Copy link
Contributor Author

Rebased, added news entry, marked ready for review.

About the test, I was unsure where to put it. Looks like there are zero tests for PipReporter, and pip._internal.resolution.resolvelib.resolver.Resolver (which uses PipReporter) only gets tested in tests/unit/resolution_resolvelib/test_resolver.py, but for candidate selection rather than messages or logs.

@astrojuanlu astrojuanlu force-pushed the present-found-conflicts-when-discarding branch from 0ee6cf9 to 1dc37f4 Compare March 26, 2023 12:31
@astrojuanlu
Copy link
Contributor Author

Test failure seems unrelated?

      ModuleNotFoundError: No module named 'platform'

@pradyunsg
Copy link
Member

Yea, ignore that failure. :)

@pradyunsg pradyunsg merged commit 9731131 into pypa:main Mar 26, 2023
@astrojuanlu astrojuanlu deleted the present-found-conflicts-when-discarding branch March 26, 2023 13:49
@astrojuanlu
Copy link
Contributor Author

Didn't expect the quick turnaround! So excited to have made my first code contribution to pip. Thanks for the patience 🙏🏽

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

request for backtracking output to say what the multiple dependencies causing conflicts are
5 participants