rez icon indicating copy to clipboard operation
rez copied to clipboard

Replace `rez.utils.data_utils.cached_property` with `functools.cached_property`

Open JeanChristopheMorinPerso opened this issue 1 year ago • 3 comments

Now that we dropped support for Python 2, we should take a look at if we can replace rez.utils.data_utils.cached_property with functools.cached_property.

rez.utils.data_utils.cached_property has multiple issues, one of which is that sphinx doesn't recognize the wrapped function as a property, see https://rez.readthedocs.io/en/stable/api/rez.system.html#rez.system.System.platform. On top of that, I don't think it's type hint friendly.

Python now has a built-in cached_property in the functools module.

Things to take into consideration:

  • Our version has an uncache method. Is it something we need?
  • Would functools.cached_property be slower or faster than what we have today? Slower would mean that we would need to find a way to make Sphinx recognize cached properties correctly.

This is at the very core of rez, so we have to be careful.

I started looking into the issue and wanted to clarify a couple of things.

  1. Does this issue mean rez will drop Python 3.7 compatibility? functools.cached_property was added in Python 3.8 (docs, StackOverflow). As an alternative, some sort of compatibility layer can be introduced (e.g., use the replacement starting with Python 3.8, and leave the previous implementation for Python 3.7). What is a preferred solution here?

  2. Do we need to be concerned about uncache? functools.cached_property uncaches a property the same way, by deleting the attribute (docs):

    The cached value can be cleared by deleting the attribute. This allows the cached_property method to run again.

    So my intuition is that whenever uncache is called in current solution, it can be reimplemented in the replacement, too.

vergeev avatar Feb 14 '24 07:02 vergeev

  1. Does this issue mean rez will drop Python 3.7 compatibility? functools.cached_property was added in Python 3.8 (docs, StackOverflow). As an alternative, some sort of compatibility layer can be introduced (e.g., use the replacement starting with Python 3.8, and leave the previous implementation for Python 3.7). What is a preferred solution here?

We could potentially depend on https://pypi.org/project/backports.cached-property/ for Python 3.7.

  1. Do we need to be concerned about uncache? functools.cached_property uncaches a property the same way, by deleting the attribute (docs):

From what I see, uncached is used in two different places (see https://github.com/search?q=repo%3AAcademySoftwareFoundation%2Frez+%2F%5B%5E_+%5Duncache%5C%28%2F&type=code). If we can delete the attribute to uncache, then I guess we could try to use the built-in cached_property.

One thing I'm very concerned about is the speed. cached_property is used in hot paths. I see a lot more attribute accesses in functools.cached_property than in our version.

Thinking about it a little bit more, maybe we don't need to get rid of our implementation. I'd be interested to see if it can provide any speed improvements though.