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

Old behavior when there is no previous_satisfier #257

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Eh2406
Copy link
Member

@Eh2406 Eh2406 commented Sep 6, 2024

In #79 we removed the special case described in the original PubGrub documentation as handling there not being a previous_satisfier. This PR brings that special case back. We believe that this special case happens when there is only one package referenced in the conflict, and that package was depended on by only one other package. Specifically under some circumstances the old code would point out that the unitary conflict has been "almost satisfied" from the very beginning of history and so backjump to beginning of the problem. (It is always technically safe to have excessive backjump. Even restarting with no decisions each time there's a conflict would still get the correct answer.) Instead after #79 they would backjump only to where the package was first depended on, triggering resolving conflict by marking the depender is unavailable.

On one hand, the post #79 code involves less backjumping and therefore less redundant decision-making and keeps the state free of unnecessary facts. On the other hand the old code put a fundamental conflict in the correct place in the state so that it will not get repeatedly removed and re-added when backjumping.

Which one is faster depends on how much optimizations we've applied to different parts of the code and the exact shape of the problem. Crates.io (without Solana) gets 14% faster. The synthetic slow_135 (based on #135) gets 19% faster. elm, "large_case", and zule have small regressions that are as likely to be noise as real. The Sudoku problem gets ~4x slower. Based on extensive testing, all of these changes are due to the algorithmic change. The second commit which reduces allocations does not have a measurable impact on performance.

By changing the order incompatibilities are discovered and combined this changes error messages. Whether for the better or worse this repo does not have sufficient testing to determine.

Some relevant back links are

@Eh2406
Copy link
Member Author

Eh2406 commented Sep 6, 2024

@konstin and @zanieb you have extensive testing of error messages, does this change make things easier or harder to understand? Similarly for your use cases / benchmarks how does it affect performance?
I will leave this PR in Draft until you have a chance to look.

@konstin
Copy link
Member

konstin commented Sep 17, 2024

I've tried this branch with the uv fork: https://github.com/astral-sh/uv/compare/konsti/pubgrub-257

A lot of messages become clearer: We're not starting with some dependencies, but with your project and its direct dependencies.

old:

× No solution found when resolving dependencies:
╰─▶ Because there are no versions of xyz and your project depends on xyz, we can conclude that your project's requirements are unsatisfiable.

new:

× No solution found when resolving dependencies:
╰─▶ Because your project depends on xyz and there are no versions of xyz, we can conclude that your project's requirements are unsatisfiable.

Here the new one feels like it skips less steps.

old:

× No solution found when resolving dependencies:
╰─▶ Because package-b==1.0.0 depends on package-c{sys_platform == 'darwin'}>=2.0.0 and only package-c{sys_platform == 'darwin'}<=2.0.0 is available, we can conclude that package-b==1.0.0 depends on package-c{sys_platform == 'darwin'}==2.0.0.
  And because only the following versions of package-c{sys_platform == 'linux'} are available:
      package-c{sys_platform == 'linux'}==1.0.0
      package-c{sys_platform == 'linux'}>2.0.0
  and package-a==1.0.0 depends on package-c{sys_platform == 'linux'}<2.0.0, we can conclude that package-a==1.0.0 and package-b==1.0.0 are incompatible.
  And because your project depends on package-a==1.0.0 and package-b==1.0.0, we can conclude that your project's requirements are unsatisfiable.

new:

× No solution found when resolving dependencies:
╰─▶ Because package-a==1.0.0 depends on package-c{sys_platform == 'linux'}<2.0.0 and only the following versions of package-c{sys_platform == 'linux'} are available:
      package-c{sys_platform == 'linux'}==1.0.0
      package-c{sys_platform == 'linux'}>2.0.0
  we can conclude that package-a==1.0.0 depends on package-c{sys_platform == 'linux'}==1.0.0. (1)

  Because package-b==1.0.0 depends on package-c{sys_platform == 'darwin'}>=2.0.0 and only package-c{sys_platform == 'darwin'}<=2.0.0 is available, we can conclude that package-b==1.0.0 depends on package-c{sys_platform == 'darwin'}==2.0.0.
  And because we know from (1) that package-a==1.0.0 depends on package-c{sys_platform == 'linux'}==1.0.0, we can conclude that package-a==1.0.0 and package-b==1.0.0 are incompatible.
  And because your project depends on package-a==1.0.0 and package-b==1.0.0, we can conclude that your project's requirements are unsatisfiable.

Neutral to slight regression: This one feels less consistent now, instead of either doing it from the end or from the beginning, we're now backtracking from the middle.

old:

× No solution found when resolving dependencies:
╰─▶ Because there is no version of anyio==0.0.0 and lib==0.0.0 depends on anyio==0.0.0, we can conclude that lib==0.0.0 cannot be used.
  And because only lib==0.0.0 is available and example==0.0.0 depends on lib, we can conclude that example==0.0.0 cannot be used.
  And because only example==0.0.0 is available and you require example, we can conclude that your requirements are unsatisfiable.

new:

× No solution found when resolving dependencies:
╰─▶ Because only example==0.0.0 is available and example==0.0.0 depends on lib, we can conclude that all versions of example depend on lib.
  And because only lib==0.0.0 is available and lib==0.0.0 depends on anyio==0.0.0, we can conclude that all versions of example depend on anyio==0.0.0.
  And because there is no version of anyio==0.0.0 and you require example, we can conclude that your requirements are unsatisfiable.

This one is imo a regression, the "all of" list is strange:

old:

× No solution found when resolving dependencies:
╰─▶ Because the requested Python version (>=3.7) does not satisfy Python>=3.8 and the requested Python version (>=3.7) does not satisfy Python>=3.7.9,<3.8, we can conclude that Python>=3.7.9 is incompatible.
  And because pygls>=1.1.0,<=1.2.1 depends on Python>=3.7.9,<4 and only pygls<=1.3.0 is available, we can conclude that all of:
      pygls>=1.1.0,<1.3.0
      pygls>1.3.0
   cannot be used. (1)

  Because the requested Python version (>=3.7) does not satisfy Python>=3.8 and pygls==1.3.0 depends on Python>=3.8, we can conclude that pygls==1.3.0 cannot be used.
  And because we know from (1) that all of:
      pygls>=1.1.0,<1.3.0
      pygls>1.3.0
   cannot be used, we can conclude that pygls>=1.1.0 cannot be used.
  And because your project depends on pygls>=1.1.0, we can conclude that your project's requirements are unsatisfiable.

new:

× No solution found when resolving dependencies:
╰─▶ Because your project depends on pygls>=1.1.0 and pygls>=1.1.0,<=1.2.1 depends on Python>=3.7.9,<4, we can conclude that all of:
      Python<3.7.9
      Python>=4
  , your project, pygls!=1.3.0 are incompatible.
  And because the requested Python version (>=3.7) does not satisfy Python>=3.7.9,<3.8, we can conclude that all of:
      Python<3.8
      Python>=4
  , your project, pygls!=1.3.0 are incompatible.
  And because pygls==1.3.0 depends on Python>=3.8 and the requested Python version (>=3.7) does not satisfy Python>=3.8, we can conclude that your project's requirements are unsatisfiable.

Overall, I like the strategy of starting from the project instead of the deepest dep more, but we should do one or the other consistently, right now we sometimes start in the middle.

@zanieb what do think

@zanieb
Copy link
Member

zanieb commented Nov 17, 2024

I'm not really sure if it's better to start with "your project depends on xyz" or with "there are no versions of xyz". On one hand, it's nice to have a logical forward progression, but it's also nice to get to the reason the resolution failed quickly. As Konsti suggested, starting in the middle is definitely not ideal.

Generally, the way that incompatibilities are ordered by PubGrub are a mystery to me and I am okay with changes. Let us know if you want us to do more testing.

Copy link

codspeed-hq bot commented Nov 19, 2024

CodSpeed Performance Report

Merging #257 will improve performances by ×40

Comparing Eh2406:pre-79 (4ce8684) with dev (8a29d57)

Summary

⚡ 1 improvements
✅ 5 untouched benchmarks

Benchmarks breakdown

Benchmark dev Eh2406:pre-79 Change
backtracking_ranges 1,976.3 ms 49.7 ms ×40

@x-hgg-x
Copy link
Contributor

x-hgg-x commented Nov 25, 2024

With this PR, resolution time for solana-archiver-lib v1.1.12 using https://github.com/x-hgg-x/pubgrub-bench is 2x slower, since with #274 spending more time to find a better satisfier is faster than redoing all the work since DecisionLevel(1).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants