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

Cache simple repository pages #6932

Merged
merged 2 commits into from
Nov 3, 2022

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Nov 1, 2022

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.

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.

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 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

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

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/sources Releated to package sources/indexes/repositories kind/refactor Pulls that refactor, or clean-up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants