pypa / packaging

Core utilities for Python packages
https://packaging.pypa.io/
Other
620 stars 248 forks source link

Does caching belong in `packaging` or `pip`? #460

Closed jbylund closed 3 years ago

jbylund commented 3 years ago

Issue to hold some higher level discussion about caching/performance and if it belongs in packaging in in consumers (pip & others).

Two prs are currently open:

458 and

459

brettcannon commented 3 years ago

I've mentioned my opinion mostly in #458 , but I don't think we should be adding caching. That's something projects will have individual needs about in terms of control over a cache and I don't think we should try and come up with an API to allow for that control. While I totally appreciate @jbylund trying to speed up pip, I think this sort of caching belongs in pip itself and not down at our layer.

@di @pradyunsg thoughts?

pradyunsg commented 3 years ago

I think I agree that this belongs in pip.

The strongest reason that I can think of is that the users of this library should make the call on what they want the characteristics of the cache to be (memory usage, LRU vs LFU etc) and it is fairly trivial to cache these objects outside of this library.

What does make sense for us here though, is optimising how this library works. That isn’t what this issue is about, but improving how much memory we use for parsing things, and possibly writing a hand-crafted parser for requirements, would be ideal.

jbylund commented 3 years ago

Just by way of example, factory:find_candidates and constructors:install_req_from_req_string (both from pip) construct a ton of requirement objects, which currently call pyparsing:parseString which is expensive (everything relatively speaking). One option would be that pip could have a wrapper class over Requirement which adds the caching behavior?

uranusjr commented 3 years ago

I'd say pip would gain a parse_requirement function that does the caching, and everything can call that instead of Requirement directly.

pradyunsg commented 3 years ago

Looks like we have agreement!