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

Cleanup backports since we are now Python 3.x+ only #1634

Conversation

BryceGattis
Copy link
Contributor

No description provided.

@BryceGattis BryceGattis requested a review from a team as a code owner February 11, 2024 21:55
@BryceGattis BryceGattis marked this pull request as draft February 11, 2024 21:56
@BryceGattis BryceGattis marked this pull request as ready for review February 12, 2024 00:29
@JeanChristopheMorinPerso
Copy link
Member

Can we really replace our lrucache with the built-in one? The one we have has a comment that says that it's modified to make it more performant.

@predat
Copy link
Contributor

predat commented Feb 15, 2024

Can we really replace our lrucache with the built-in one? The one we have has a comment that says that it's modified to make it more performant.

As far as I know, lru_cache has had a C implementation since at least python 3.7. I've done a few tests of my own, and functools.lru_cache seems to be at least 3x faster than rez.backport.lru_cache.lru_cache with python 3.11.

@predat
Copy link
Contributor

predat commented Feb 15, 2024

Do you think this is a good place to also tackle enum ?

@JeanChristopheMorinPerso
Copy link
Member

Can we really replace our lrucache with the built-in one? The one we have has a comment that says that it's modified to make it more performant.

As far as I know, lru_cache has had a C implementation since at least python 3.7. I've done a few tests of my own, and functools.lru_cache seems to be at least 3x faster than rez.backport.lru_cache.lru_cache with python 3.11.

Can you share the code you used to run your benchmark? We can also run rez's own benchmark (because yes we have benchmarks).

Do you think this is a good place to also tackle enum
@predat enum should go in another PR IMO.

@predat
Copy link
Contributor

predat commented Feb 16, 2024

Can you share the code you used to run your benchmark? We can also run rez's own benchmark (because yes we have benchmarks).

@lru_cache(maxsize=1000)
def fib(n):
    return 1 if n in (0, 1) else fib(n - 1) + fib(n - 2)

with from rez.backport.lru_cache import lru_cache:

python -m timeit -s 'from fib import fib' 'fib(495)'
500000 loops, best of 5: 860 nsec per loop

with from functools import lru_cache:

python -m timeit -s 'from fib import fib' 'fib(495)'
5000000 loops, best of 5: 53.9 nsec per loop

Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oups, I wanted to mark it as "request changes". @BryceGattis can you rebase/merge also to resolve the conflict?

@JeanChristopheMorinPerso JeanChristopheMorinPerso added the run-benchmarks Add this label to a PR to run the benchmarks label Feb 17, 2024
Signed-off-by: Bryce Gattis <brycegattis@yahoo.com>
Signed-off-by: Bryce Gattis <brycegattis@yahoo.com>
Signed-off-by: Bryce Gattis <brycegattis@yahoo.com>
Signed-off-by: Bryce Gattis <brycegattis@yahoo.com>
Signed-off-by: Bryce Gattis <brycegattis@yahoo.com>
…g.py so this PR doesn't affect the vendor folder

Signed-off-by: Bryce Gattis <brycegattis@yahoo.com>
@BryceGattis
Copy link
Contributor Author

Restored rez/backport/ordereddict.py so it can still be used the src/rez/vendor/pyparsing/pyparsing.py vendor file. 👍

@JeanChristopheMorinPerso
Copy link
Member

@BryceGattis you can remove rez/backport/ordereddict.py since it's not used. The code in rez.vendor.pyparsing.pyparsing was not adjusted by us, see https://github.com/pyparsing/pyparsing/blob/adf8dd00b736ba1934914e1db78528af02662b65/pyparsing.py#L133-L139.

Signed-off-by: Bryce Gattis <brycegattis@yahoo.com>
@BryceGattis
Copy link
Contributor Author

@JeanChristopheMorinPerso Notes addressed 👍

@JeanChristopheMorinPerso JeanChristopheMorinPerso merged commit 8d56142 into AcademySoftwareFoundation:main Mar 1, 2024
52 checks passed
Pixel-Minions added a commit to Pixel-Minions/rez that referenced this pull request Sep 26, 2024
Signed-off-by: Bryce Gattis <brycegattis@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-benchmarks Add this label to a PR to run the benchmarks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants