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

Solver performance #5335

Merged
merged 4 commits into from
Apr 8, 2022
Merged

Solver performance #5335

merged 4 commits into from
Apr 8, 2022

Conversation

dimbleby
Copy link
Contributor

Some performance improvements derived from investigating #5217.

At time of writing, poetry install localstack['runtime'] is unusably slow.
What happens is something like this:

  • that package has a dependency on pyyaml>=5.1, which poetry resolves early on
    by picking pyyaml 6.0
  • that package also has a dependency on awscli>=1.14.18
  • there are a huge number of versions of awscli satisfying that constraint,
    every single one of them depending on a version of pyyaml that is older than
    6.0.

Poetry spends absolutely forever trying to find a compatible version of awscli,
and failing.
Eventually it will decide that they all fail, backtrack, and get to a solution -
but not fast!

(It could be more complicated than that: I think that there may be more than one
package playing the pyyaml role in this story.)

The first commit in this MR just tidies up a testcase that I encountered going
wrong while working out a fix.
The testcase says that cachy at ^0.1.0 was requested and that 0.1.0.dev1 was
installed.
But that would never happen, because poetry treats 0.1.0.dev1 as being earlier
than 0.1.0.

The second commit borrows from pubgrub-rs/pubgrub#88
to considerably speed up propagation of incompatibilities.

The third commit improves our repeated searches for suitable packages.
During the search, once a package has been rejected it can only become a valid
candidate again if we backtrack.
Since we build up constraints linear in size with the number of rejected
packages - like >=1.14.18,<1.14.19 || >1.14.19,<1.14.20 || >1.14.20,... - the
repeated work to reject non-matching packages becomes expensive.
I've avoided that by introducing a cache that discards packages as we go.

The fourth commit puts a cache on some Term methods that were showing up as
expensive.
(In spite of this, Term.satisfies() remains prominent in the profiling.
Probably these two caches should be seen as a sticking plaster trying - but not
fully succeeding - to compensate for something expensive in the algorithm.)

I have mostly been testing with a pyproject.toml that contains the following two
constraints:

awscli = ">=1.19.0"
localstack = { version = "0.14.1.1", extras = ["runtime"] }

where the first of these is to keep a cap on the number of awscli packages
that we must check.

The fixes in this MR take solution of the above from about 11 minutes to about 1
minute for me.
(Not counting download time, I'm testing with a populated cache.)

That's still a long time: and removing the awscli line so that we have only the
localstack requirement, we still take longer than I was willing to wait to get
to a solution...

These fixes all should be clear improvements; but I don't have a good sense for
whether the things that they address are relatively common, or rare.

@dimbleby dimbleby force-pushed the solver-performance branch 2 times, most recently from ac1bce2 to 1bc2959 Compare March 20, 2022 16:11
@abn abn requested a review from a team March 22, 2022 19:19
@abn abn added area/solver Related to the dependency resolver kind/enhancement Not a bug or feature, but improves usability or performance labels Mar 22, 2022
Copy link
Member

@abn abn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the general idea. Left some high level comments for now.

src/poetry/mixology/version_solver.py Show resolved Hide resolved
src/poetry/mixology/version_solver.py Show resolved Hide resolved
@abn abn merged commit cc4d689 into python-poetry:master Apr 8, 2022
@dimbleby dimbleby deleted the solver-performance branch April 8, 2022 16:45
@abn abn mentioned this pull request Jun 6, 2022
@mm-matthias
Copy link

Related to #5896.

@mm-matthias
Copy link

I used 1.2.0b2 to check if this PR solves the cases mentioned in #5896 and the duplicated/similar tickets. While everything seems to run a bit faster poetry still tries to evaluate every single one of the awscli versions. Usually this means downloading every single package from pypi first which makes a single update run take hours, because there are thousands of awscli versions.
To make these cases significantly faster poetry has to avoid evaluating thousands of individual package versions in the first place. After all if I give a manual hint to the package version as outlined in #5896 there is actually a valid solution that is reached in a few seconds. It's just that the current solver does not seem smart enough to avoid evaluating solutions that involve a few thousand package versions.

@dimbleby
Copy link
Contributor Author

It is only hindsight that could possibly allow poetry to know that all of those thousands of awscli versions are not compatible with the current state of the search. During the search, it has no choice but to check.

Regardless of any potential future optimisations, specifying a recent lower bound for awscli is always likely to be the best way to guide poetry in the right direction. If you encounter packages that don't do this (and therefore trigger the painful case), do consider asking them if they wouldn't mind making the change eg as localstack did in localstack/localstack#5912

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/solver Related to the dependency resolver kind/enhancement Not a bug or feature, but improves usability or performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants