Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Tracked by #1635
JeanChristopheMorinPerso opened this issue Feb 12, 2024 · 3 comments
Labels
difficulty/hard This task is hard enhancement help wanted Help us improve rez

Comments

@JeanChristopheMorinPerso
Copy link
Member

JeanChristopheMorinPerso commented Feb 12, 2024

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.

@JeanChristopheMorinPerso JeanChristopheMorinPerso changed the title Remove rez.utils.data_utils.cached_property and rely on [functools.cached_property](https://docs.python.org/3/library/functools.html#functools.cached_property)? Replace rez.utils.data_utils.cached_property with functools.cached_property Feb 12, 2024
@vergeev
Copy link
Contributor

vergeev commented Feb 14, 2024

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
Copy link
Member Author

  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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty/hard This task is hard enhancement help wanted Help us improve rez
Projects
None yet
Development

No branches or pull requests

2 participants