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

Caching results of PackageInfo._pep517_metadata to cut lock time by 85%? #5477

Closed
2 tasks done
sneakers-the-rat opened this issue Apr 21, 2022 · 5 comments · Fixed by #5601
Closed
2 tasks done

Caching results of PackageInfo._pep517_metadata to cut lock time by 85%? #5477

sneakers-the-rat opened this issue Apr 21, 2022 · 5 comments · Fixed by #5601
Labels
kind/feature Feature requests/implementations

Comments

@sneakers-the-rat
Copy link
Contributor

  • I have searched the issues of this repo and believe that this is not a duplicate.
  • I have searched the documentation and believe that my question is not covered.

Feature Request

On poetry 1.2.0b.

Profiled the lock routine, and it spends almost all of its time,( in this lock, 217 out of 260 total seconds) in this method:

def _pep517_metadata(cls, path: Path) -> PackageInfo:

which is understandable because it needs to install the package. But in my case it ends up installing the same versions of the package multiple times (eg. I have different dependencies for pandas depending on python version, and then for each version of pandas it has to solve the same dependencies for numpy depending on arch), and then across multiple locks the problem really compounds.

Shouldn't the result be the same every time you install the package? Why not cache this (and maybe offer a "clear/don't use installed metadata cache" if that causes problems for people")? I just did an extremely brutal hack of just pickling it within that function and loading it at the beginning if it was present and it seemed to work...

@sneakers-the-rat sneakers-the-rat added kind/feature Feature requests/implementations status/triage This issue needs to be triaged labels Apr 21, 2022
@abn
Copy link
Member

abn commented Apr 22, 2022

@sneakers-the-rat If you have variations of a dependency all pointing at a path dependency, then I suspect the inspection is done each time. You are correct that this can be cached, however only within a single run. Caching it on disk might not be recommended as if the path dependency has a version/dependency change, we will get incorrect infomation.

Maybe a easy win here is to add @cache to the method.

@sneakers-the-rat
Copy link
Contributor Author

It seems like it would be possible to invalidate the cache by comparing hashes, no? like have the output of this function tied to a hash of the tarfile it's made from and if that changes then invalidate? i've spent a while trying to get my head around this library but admittedly still dont' quite understand it. I can try using @cache, is that something y'all would take a PR for?

@abn
Copy link
Member

abn commented Apr 23, 2022

I would say we start with the cache. The issue is that _pep517_metadata is also used with directory paths, ie. it is not always related to tar files. But also sometimes local path dependencies (eg: vcs dependencies) etc.

It would be great if we can add some testing in there too if workabe, but I suspect other cases should cover this as well.

@sneakers-the-rat
Copy link
Contributor Author

great, I'll take a look at the existing tests, how @cache is used, and how _pep517_metadata is called and give it a shot.

@mkniewallner mkniewallner removed the status/triage This issue needs to be triaged label Jun 11, 2022
Copy link

github-actions bot commented Mar 1, 2024

This issue 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 Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/feature Feature requests/implementations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants