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

Add caching for properties in VersionClassBase #292

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AbdealiLoKo
Copy link
Contributor

@AbdealiLoKo AbdealiLoKo commented Aug 24, 2022

Resolves #290

Improve performance in properties of VersionClassBase
So that they don't do extra sql queries and we can reuse the existing values

@marksteward
Copy link
Collaborator

I'm really not convinced this is suitable. What makes it hard for you to cache the property yourself?

@AbdealiLoKo
Copy link
Contributor Author

Just a better developer experience.

As a user - I was surprised to see that this is generating a query every time.
And I felt like developers using sqla-continuum would not realize this until they enable sqlalchemy echo=True
(For example - When I use sqlalchemy - I expect that it caches the relationships.)
In fact, I thought that this was just a relationship until I realized checked that it was not!

If you think it's not worth it to add to the library I am fine with it - we can close the issue and PR.
I do have a workaround I was able to create:

class VersionClassCustom:
    @cached_property
    def previous(self):
        return super().previous

    @cached_property
    def next(self):
        return super().next

    @cached_property
    def index(self):
        return super().index

from sqlalchemy_continuum import make_versioned
from sqlalchemy_utils.functions import get_declarative_base
from myapp.models import MyModel

make_versioned(
    options={
        'base_classes': (VersionClassCustomBase, get_declarative_base(MyModel)),
    },
)

@marksteward
Copy link
Collaborator

marksteward commented Aug 24, 2022

That's a good point. However, it's a potentially breaking change so would need to be in a major release.

I guess I'm surprised that you need to call .next or .previous multiple times for a version. What sort of use case is it? A template?

@AbdealiLoKo
Copy link
Contributor Author

We have a stringification logic which takes a version and tries to explain in english what changes have occured (So its kinda like a template - yes)
We have multiple functions - handling columns, manytomany/onetomany/etc. relationships, association-proxies, column_properties
To each of these functions we pass the version and use version.previous to get the previous version

I don't mind it being in a major release cause i do have a workaround which is elegant.
Should I be doing something to mark this as a breaking change ? Or just leave the PR as is ?

@marksteward
Copy link
Collaborator

marksteward commented Aug 24, 2022

I haven't given much thought to a major release, as I'm focusing on maintenance for now and this is the first thing that would be in it. It would be good to get tests added specifically for this feature.

I'd then be happy to approve it and see if anyone else has feedback on the issue or PR. Then maybe merge it in a few weeks? If you have any other improvements (e.g. #286) maybe we can get them all into one release.

@AbdealiLoKo
Copy link
Contributor Author

Sure - sounds good

@marksteward
Copy link
Collaborator

Can you rebase this? Thanks!

Cache some properties in VersionClassBase as it won't really change
and can improve performance significantly by avoiding extra queries

Tested this in my own application and also in a shell:
>>> class A:
...     @cached_property
...     def abc(self):
...         print("test")
...         return 1
...
>>> a = A()
>>> a.abc
test
1
>>> a.abc
1
@AbdealiLoKo AbdealiLoKo force-pushed the ajk-cache branch 2 times, most recently from 39b57fe to e93cde3 Compare August 29, 2022 10:25
@AbdealiLoKo
Copy link
Contributor Author

AbdealiLoKo commented Aug 29, 2022

Rebased and found a minor issue - fixed it.
Tests pass \^_^/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Caching the version.previous attribute
2 participants