Skip to content

Comments

poc: replace cachy with cache module#5868

Merged
neersighted merged 2 commits intopython-poetry:masterfrom
chadac:feature/cachy-migration
Oct 11, 2022
Merged

poc: replace cachy with cache module#5868
neersighted merged 2 commits intopython-poetry:masterfrom
chadac:feature/cachy-migration

Conversation

@chadac
Copy link
Contributor

@chadac chadac commented Jun 18, 2022 •
edited
Loading

Related to the discussion in #5720.

This is a test to see what would be needed to pull out of cachy to get it into Poetry entirely. The result is a new utils module, poetry.utils.cache, that is about 250 LOC. I also went ahead and made a couple of small improvements:

  • Broke up cachy into simpler classes, mainly to push stuff out of config and into typed variables. Now caches are declared separately -- FileCache, DictCache
  • Add types to methods and generics
  • Update the file format. Before it was the expiration time concatenated with the payload, now it's purely JSON encoded. Added compatibility tests and checks to ensure the migration won't break existing caches.

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.

@chadac chadac changed the title poc: Replace cachy with cache module poc: replace cachy with cache module Jun 18, 2022
danieleades
danieleades reviewed Jun 18, 2022
View reviewed changes
@chadac chadac marked this pull request as ready for review June 18, 2022 18:41
chadac
chadac commented Jun 18, 2022
View reviewed changes
abn
abn previously requested changes Jun 20, 2022
View reviewed changes
@abn
Copy link
Member

abn commented Jun 20, 2022

Have not gone through the cache logic itself, but left some high level comments regarding the change.

@chadac chadac force-pushed the feature/cachy-migration branch from 149dcf8 to 8f9c039 Compare July 19, 2022 19:07
@Secrus Secrus requested review from abn and neersighted July 25, 2022 09:07
@dimbleby dimbleby mentioned this pull request Aug 13, 2022
Closed
2 tasks
@Secrus
Copy link
Member

Secrus commented Aug 26, 2022

@chadac any progress on this?

@chadac
Copy link
Contributor Author

chadac commented Sep 13, 2022

@Secrus Apologies, got pulled into some busy separate stuff that has occupied my time. I'll commit to get some of those profiling stats up for next week so to help reach a decision on one of those threads above (for DictCache).

@neersighted neersighted added this to the 1.3 milestone Oct 10, 2022
@neersighted neersighted added kind/refactor Pulls that refactor, or clean-up code area/sources Releated to package sources/indexes/repositories labels Oct 10, 2022
@chadac chadac force-pushed the feature/cachy-migration branch from 56bb897 to 360f8a2 Compare October 10, 2022 17:26
@chadac
Copy link
Contributor Author

chadac commented Oct 10, 2022

Posted an update which cuts down a lot of the junk. I did do a test on the proposed match and package caches referenced above. For PyPiRepository I found relatively little difference, so I focused testing on LegacyRepository with a sample package of about 10 packages. With caching, average install time took about 55.2s +/- 1.5s, while without caching it took around 56.1s +/- 2.7s. This was for a sample size of 10, each run inside a Docker container under the same conditions. Since this is well within the margin of error I believe it's not a significant difference, but I'm increasing the sample size now to increase confidence. I'll get a proper hypothesis test once ready to officially confirm.

neersighted
neersighted requested changes Oct 11, 2022
View reviewed changes
@chadac chadac force-pushed the feature/cachy-migration branch from 894d70c to 05130ea Compare October 11, 2022 14:40
@chadac
Copy link
Contributor Author

chadac commented Oct 11, 2022

I did end up running a more complete performance comparison for the package & match cache. The results mostly agreed with what was shown earlier; when comparing 50 identical runs between the two, the improvement in performance was about 0.5%, which was well within the margin of error (difference in average: 0.3s, with standard deviation of +/- 3.2s). The student t test reports a p-value of around 0.6, indicating that there was very little certainty that this adds any improvement at all.

neersighted
neersighted requested changes Oct 11, 2022
View reviewed changes
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.

Looks really good to me -- could you address the last review comments and then squash history into something mergable?

e.g. two commits: introduce FileCache and tests, then cut over to the new FileCache

@chadac
Copy link
Contributor Author

chadac commented Oct 11, 2022

Sounds good, lemme refactor the commits.

@chadac chadac force-pushed the feature/cachy-migration branch from beae7fe to 6f74773 Compare October 11, 2022 18:56
@neersighted neersighted force-pushed the feature/cachy-migration branch from 6f74773 to 0136c33 Compare October 11, 2022 19:27
@neersighted neersighted force-pushed the feature/cachy-migration branch from 0136c33 to a7e5f9a Compare October 11, 2022 20:53
@neersighted neersighted enabled auto-merge (rebase) October 11, 2022 20:54
@neersighted neersighted merged commit c2b1fb3 into python-poetry:master Oct 11, 2022
@chadac chadac deleted the feature/cachy-migration branch October 11, 2022 21:08
@dimbleby dimbleby mentioned this pull request Nov 1, 2022
Merged
neersighted pushed a commit that referenced this pull request Nov 3, 2022
@dimbleby
feat: cache simple repository pages (#6932)
0dc3d54 
At #5868, I
[suggested](#5868 (comment))
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.
@radoering radoering mentioned this pull request Dec 22, 2022
Merged
This was referenced Jan 30, 2023
Merged
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

@abn abn Awaiting requested review from abn

+2 more reviewers

@dimbleby dimbleby dimbleby left review comments

@danieleades danieleades danieleades left review comments

Reviewers whose approvals may not affect merge requirements

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.

6 participants

@chadac @abn @Secrus @dimbleby @neersighted @danieleades