Make `cache` support methods

[brandonwillard]

Currently, the cache function doesn't truly support methods (see #1129). Let's consider

• making the cache interface private/internal,
• adding support for this (and admitting that it's a public interface/utility), or—at the very least
• adding a warning to prevent confusion.

[brandonwillard]

I noticed from the helpful test case in #1129—and the relevant vllm code—that the cache decorator is being directly applied to the function object, so we may not even be able to tell when a function is a method in order to give a warning. We might be able to handle the case of a method descriptor object, but that wouldn't have helped in the examples above.

In other words, there's probably not a general way to warn people that they could be doing something undesirable for caching. Because of that, we might want to make the caching utilities private and leave it up to the user to determine how caching should work, since all we can really guarantee is that underlying objects are serializable in a way that can work for caching.

[Quexington]

I did some research here and it seems like the reason the test case in the PR is failing is because the function being pickled for the cache has a different closure environment. The fact that store == [] for the first cache check and store == [a_1] for the second cache check means that the pickle serializations are different as indeed they should be. Consider this minimal example which does pass:

store = []

def update_store(a):
    store.append(a)

def test_get_cache_from_class_method(test_cache):
    class DummyObject:
        @classmethod
        @test_cache
        def dummy_function(cls, a):
            update_store(a)

    DummyObject.dummy_function(1)
    DummyObject.dummy_function(1)

    assert len(store) == 1

since store is not acquired via closure, the environment for both invocations of the function is the same and therefore the cache is actually hit and the side effect is prevented the second time around.

Unfortunately, I can't identify why the relevant vllm code is suffering from this since it's not using any closures. But the takeaway should be that actually cloudpickle does work for classmethods, perhaps even better than one might suppose.

If a warning is still desirable, you might want to trigger it by checking if len(cached_function.__closure__) > 0 but I'm not 100% sure a warning is warranted here since it seems like everything is being handled correctly; the cache misses seem to be intentional and good.