Remove lru cache from methods [enable ruff rule cached-instance-method (B019)]

[notatallshaw]

I've been slowly reviewing all ruff rules. I noticed B019 rule is disabled: https://docs.astral.sh/ruff/rules/cached-instance-method/

Upon reviewing I consider it to be a correct rule, you can accidentally keep an instance of an object alive for much longer than expected, if that instance is itself referencing an object holding on to a large amount of memory this can lead to using a lot more memory than expected.

Upon removing the lru_cache for PackageFinder.find_best_candidate it revealed that some wrong types were being passed to it in PackageFinder.find_requirement as InstallRequirement.name can be None. I think it's correct to simply assert here that req.name isn't None, otherwise you end up down a rabbit hole of adding extra Optional types and None handling.

This PR makes no appreciable memory saving gain in any of the tests I ran, it's more of a preventative measure from someone incorrectly doing this in the future. I'll understand if reviewers think the refactoring is a little too complex here. I was debating whether to submit this at all after finding no performance improvement, but I do think this rule is correct.

[ichard26]

Would a staticmethod work instead as the instance isn't used here? I'm not a huge fan of breaking this out into a helper function

[notatallshaw]

Agreed, and I used this hacky code to assure myself this works as expected:

class C:
    def __init__(self): ...

    @staticmethod
    @lru_cache
    def compute(): return 1

    def __del__(self):
        print("C destructor called")

c = C()
c.compute()
del c
print("Deleted c")

The destructor call should happen before the "Deleted c" is printed, which does not happen if compute is a regular method and lru_cache is used.

[ichard26]

Drive-by review. I'm not making a comment on the rule itself yet.

[ichard26]

The rule is fine, but I don't think they're applicable to most of the classes changed here. There are not going to be multiple instances of the resolvelib provider and package finder classes. In fact, they will be long-lived instances that survive to probably nearly the end of pip execution. The change to the candidate and the staticmethod change are the only changes I don't take issue with (in addition to the bad types, of course).

[notatallshaw]

I'm fairly sure there are multiple instances of FoundCandidates.

But yes, for the other instances, they are fairly long lived, and probably one instance, but that doesn't mean that the lifetime should be kept till the very end of the process.

Nor does it mean they will always be long lived with one instance, hypothetically if the build isolation became in-process there will be multiple instances of these classes that should only live for the life of any particular build isolation.