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

fix: performance regression when parsing links from legacy repositories #6442

Merged
merged 4 commits into from
Sep 9, 2022

Conversation

radoering
Copy link
Member

Resolves: #6436

Measurements of poetry lock with warm cache with example pyproject.toml from #6436:

test case time in s peak memory usage in MB
legacy repository (before) 422 113
legacy repository (after) 3 118
pypi repository 1 92

Usage of @cached_property would be nice, but unfortunately is not available in Python 3.7 and @property combined with @cached as proposed in https://docs.python.org/3/library/functools.html#functools.cached_property results in memory leaks (flake8-bugbear B019).

@neersighted neersighted added area/solver Related to the dependency resolver impact/backport Requires backport to stable branch impact/changelog Requires a changelog entry backport/1.2 labels Sep 7, 2022
@MasterNayru
Copy link
Contributor

This is like what I did but way better. Woohoo! I am so happy to have been even the slightest bit helpful in making Poetry a better tool for everyone.

I have run this against other projects that I was seeing performance issues with internally and can see that performance in dependency resolution has dramatically improved.

I can't stress enough the importance of having this performance issue fixed and in a release branch (whether it be this PR or some other one). The performance drop with the logic as it stands on v1.2.0 just makes using poetry in environments where a secondary repo is used practically unusable. I have had two backends show this same performance issue, one being pypicloud which seems to direct clients to pypi.org when it does not have a package, and AWS CodeArtifact, which can be configured to pull packages from pypi.org itself. I would just hate to have people advocate against tools like this or be hesitant to upgrade.

@radoering radoering force-pushed the legacy-repo-performance branch 2 times, most recently from f3065b9 to 738723c Compare September 8, 2022 19:12
@radoering
Copy link
Member Author

As proposed by @neersighted on discord, I added backports.cached-property in order to support cached_property on Python 3.7. This simplifies things a bit. It's in a separate commit, so we can decide if it's worth it.

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

LGTM overall, I have high confidence in backporting this -- one minor nit that we can skip if you don't like it.

src/poetry/repositories/link_sources/html.py Outdated Show resolved Hide resolved
@neersighted neersighted changed the title Fix performance regression legacy repositories fix: performance regression when parsing links from legacy repositories Sep 8, 2022
neersighted
neersighted previously approved these changes Sep 8, 2022
@neersighted neersighted enabled auto-merge (squash) September 8, 2022 20:26
@radoering
Copy link
Member Author

pre-commit.ci autofix

@neersighted neersighted merged commit c4b2253 into python-poetry:master Sep 9, 2022
@poetry-bot
Copy link

poetry-bot bot commented Sep 9, 2022

The backport to 1.2 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.2 1.2
# Navigate to the new working tree
cd .worktrees/backport-1.2
# Create a new branch
git switch --create backport-6442-to-1.2
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c4b2253793cd6b41a99e25e479e40b776cca0a0e
# Push it to GitHub
git push --set-upstream origin backport-6442-to-1.2
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.2

Then, create a pull request where the base branch is 1.2 and the compare/head branch is backport-6442-to-1.2.

radoering added a commit to radoering/poetry that referenced this pull request Sep 9, 2022
…es (python-poetry#6442)

Resolves: python-poetry#6436

Measurements of `poetry lock` with warm cache with example
pyproject.toml from python-poetry#6436:

|test case|time in s|peak memory usage in MB|
|---|---|---|
|legacy repository (before)|422|113|
|legacy repository (after)|3|118|
|pypi repository|1|92|

`backports.cached-property` is used in order to support
cached_property on Python 3.7.

Co-authored-by: Jarrod Moore <jmo@jmo.name>
Co-authored-by: Bjorn Neergaard <bjorn@neersighted.com>

(cherry picked from commit c4b2253)
radoering added a commit to radoering/poetry that referenced this pull request Sep 9, 2022
…es (python-poetry#6442)

Resolves: python-poetry#6436

Measurements of `poetry lock` with warm cache with example
pyproject.toml from python-poetry#6436:

|test case|time in s|peak memory usage in MB|
|---|---|---|
|legacy repository (before)|422|113|
|legacy repository (after)|3|118|
|pypi repository|1|92|

`backports.cached-property` is used in order to support
cached_property on Python 3.7.

(cherry picked from commit c4b2253)

Co-authored-by: Jarrod Moore <jmo@jmo.name>
Co-authored-by: Bjorn Neergaard <bjorn@neersighted.com>
radoering added a commit to radoering/poetry that referenced this pull request Sep 9, 2022
…es (python-poetry#6442)

Resolves: python-poetry#6436

Measurements of `poetry lock` with warm cache with example
pyproject.toml from python-poetry#6436:

|test case|time in s|peak memory usage in MB|
|---|---|---|
|legacy repository (before)|422|113|
|legacy repository (after)|3|118|
|pypi repository|1|92|

`backports.cached-property` is used in order to support
cached_property on Python 3.7.

(cherry picked from commit c4b2253)

Co-authored-by: Jarrod Moore <jmo@jmo.name>
Co-authored-by: Bjorn Neergaard <bjorn@neersighted.com>
neersighted added a commit that referenced this pull request Sep 10, 2022
…es (#6442)

Resolves: #6436

Measurements of `poetry lock` with warm cache with example
pyproject.toml from #6436:

|test case|time in s|peak memory usage in MB|
|---|---|---|
|legacy repository (before)|422|113|
|legacy repository (after)|3|118|
|pypi repository|1|92|

`backports.cached-property` is used in order to support
cached_property on Python 3.7.

(cherry picked from commit c4b2253)

Co-authored-by: Jarrod Moore <jmo@jmo.name>
Co-authored-by: Bjorn Neergaard <bjorn@neersighted.com>
@neersighted neersighted added this to the 1.3 milestone Sep 17, 2022
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 impact/backport Requires backport to stable branch impact/changelog Requires a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependency resolution way slower for packages with large number of releases from legacy secondary repositories
4 participants