Skip to content

Comments

Cache simple repository pages#6932

Merged
neersighted merged 2 commits intopython-poetry:masterfrom
dimbleby:cache-legacy-repository
Nov 3, 2022
Merged

Cache simple repository pages#6932
neersighted merged 2 commits intopython-poetry:masterfrom
dimbleby:cache-legacy-repository

Conversation

@dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Nov 1, 2022 •
edited
Loading

At #5868, I suggested that some of the caching that was being reworked could be removed altogether, because cachecontrol was already taking care of it just fine.

But now I find myself using an Azure artifacts repository, and it is returning headers that insist that the client does not do any caching:

cache-control: no-cache
pragma: no-cache           
x-cache: CONFIG_NOCACHE

(pypi, by contrast, sets max-age to 10 minutes here).

So I was wrong! And now I am seeing a big performance hit in some projects where the solve involves overrides and backtracking: and therefore hitting the legacy simple API repeatedly.

However, we don't need all the mechanism of cachy and its like for this, a well-placed @lru_cache() seems more than sufficient.

This makes me wonder whether it wouldn't be better to do similar for pypi anyway, and rip out cachecontrol altogether. But let's keep it simple for now: this is an easy fix to a performance regression.

@dimbleby
Copy link
Contributor Author

dimbleby commented Nov 1, 2022

  • flake8-bugbear doesn't like lru_cache on a method because it thinks we might be leaking memory
  • mypy doesn't like the self.method = lru_cache()(self._method) trick in a subclass where the parent already defines self.method

keeping everything happy is kind of annoying: I've refactored so that .get_page() rather than ._get_page() is the preferred, caching, method.

neersighted
neersighted approved these changes Nov 3, 2022
View reviewed changes
Copy link
Member

@neersighted neersighted left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is the same way we handled get_repository_config_for_url caching, so I am more than fine with the approach. Ideally we'll move to functools.cache instead of lru_cache once we eventually, in the distant future drop 3.8 support; however I think pyupgrade can likely be taught to make that change instead of leaving a comment.

@neersighted neersighted enabled auto-merge (squash) November 3, 2022 04:48
@neersighted neersighted force-pushed the cache-legacy-repository branch from c032dde to 5e4e566 Compare November 3, 2022 04:48
@neersighted neersighted added this to the 1.3 milestone Nov 3, 2022
@neersighted neersighted added kind/refactor Pulls that refactor, or clean-up code area/sources Releated to package sources/indexes/repositories impact/changelog Requires a changelog entry labels Nov 3, 2022
@neersighted neersighted merged commit 0dc3d54 into python-poetry:master Nov 3, 2022
@dimbleby dimbleby deleted the cache-legacy-repository branch November 3, 2022 09:55
@radoering radoering removed the impact/changelog Requires a changelog entry label Dec 2, 2022
@radoering
Copy link
Member

radoering commented Dec 2, 2022

I removed the changelog label because this fixes a performance regression of an unreleased version (if I understand correctly).

@radoering radoering mentioned this pull request Dec 22, 2022
Merged
@github-actions
Copy link

github-actions bot commented Feb 29, 2024

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.

Reviewers

@neersighted neersighted neersighted approved these changes

Assignees

No one assigned

Labels

area/sources Releated to package sources/indexes/repositories kind/refactor Pulls that refactor, or clean-up code

Projects

None yet

Milestone

1.3

Development

Successfully merging this pull request may close these issues.

3 participants

@dimbleby @radoering @neersighted