AcademySoftwareFoundation / rez

An integrated package configuration, build and deployment system for software
https://rez.readthedocs.io
Apache License 2.0
938 stars 332 forks source link

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

Open JeanChristopheMorinPerso opened 7 months ago

JeanChristopheMorinPerso commented 7 months ago

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:

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

vergeev commented 7 months ago

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.

JeanChristopheMorinPerso commented 7 months ago
  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.

JeanChristopheMorinPerso commented 6 months ago

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.