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

__getattr__ is not IDE friendly #3

Open
amirsoroush opened this issue Feb 24, 2024 · 8 comments
Open

__getattr__ is not IDE friendly #3

amirsoroush opened this issue Feb 24, 2024 · 8 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@amirsoroush
Copy link
Contributor

amirsoroush commented Feb 24, 2024

def __getattr__(self, name: str):

Although we simplified the implementation by delegating the unknown attributes to strategies, IDEs are not going to help us to detect problems or code completion.

Do you have any plan to address this issue? like adding if TYPE_CHECKING: block or something?

Another thing to note is that we're iterating over strategies two times, once in __getattr__ and again in the inner method. So the option is defining those methods in the PyInMemStore class itself.

@hamidrabedi hamidrabedi added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Feb 25, 2024
@hamidrabedi
Copy link
Owner

Not happy with the dynamic way of doing this, makes it hard to debug and trace as well. but couldn't find a better way to add other datatypes this easily. Don't know how to make it more efficient.

Can you explain how type checking helps?

@amirsoroush
Copy link
Contributor Author

I'm thinking too.

If we were assured that we only have those four strategies(String, Set, List, Sorted), we could define a type checking block and leave the __getattr__ as it is like:

from typing import TYPE_CHECKING

class PyInMemStore:
    def __init__():
        ...
        self.strategies = [
            StringStrategy(),
            ListStrategy(),
            SetStrategy(),
            SortedSetStrategy(),
        ]

    def __getattr__(self, name: str):
        # body
        pass

    if TYPE_CHECKING:
        def lpush(self, store: Dict[str, Any], key: str, value: Any) -> int: ...
        def rpop(self, store: Dict[str, Any], key: str) -> Any: ...

And we'll get IDE support.

But what if someone wants to create and add his/her own strategy and append it to self.strategies list? With current implementation it should just work, but without any IDE support. Also __getattr__ is a bit slow right now.


Another option is to remove __getattr__ and define the strategies like:

class PyInMemStore:
    def __init__():
        ...
        self.string: StringStrategy = StringStrategy()
        self.list: ListStrategy = ListStrategy()
        self.set: SetStrategy = SetStrategy()
        self.sorted: SortedSetStrategy = SortedSetStrategy()

users need to know the specific strategy before accessing its method which is less flexible but everything works fine and we also have type hints defined in the definitions.


We could also simplify the interface by defining those methods in the PyInMemStore itself and delegate them to the relevant strategy:

class PyInMemStore:
    def __init__():
        ...
        self.string: StringStrategy = StringStrategy()
        self.list: ListStrategy = ListStrategy()
        self.set: SetStrategy = SetStrategy()
        self.sorted: SortedSetStrategy = SortedSetStrategy()

    def lpush(self, store: Dict[str, Any], key: str, value: Any) -> int:
        return self.list.lpush(store, key, value)

They are all have cons and pros. What's your opinion?

@hamidrabedi
Copy link
Owner

I think the best approach is your first solution, That way we can keep the code dynamic and easy to add other datatypes with their own unique features.

And I'm still a newbie for type checking but could you please look at this numpy sample:
https://github.com/numpy/numpy/blob/main/numpy/ma/core.pyi

They have a '.pyi' file next to the original file, which performs type checking.
Do you think that approach is a good way of doing it? does it work?

@hamidrabedi
Copy link
Owner

hamidrabedi commented Feb 26, 2024

@amirsoroush
And wdym by:

Also __getattr__ is a bit slow right now.

All the functions are static and right now its not more that 10-15 of them, why would it be slow?
even if we increase the iteration to 100-500 it will work just fine
am i missing sth here?

@amirsoroush
Copy link
Contributor Author

Do you think that approach is a good way of doing it? does it work?

Sure it does. They're called "stub files". Mostly they're being used when we have extension modules written in C (or others, which don't have annotations) or for old modules written in Python 2. It's your call but I'm more into keeping the type hints along with the source code. There are other modules that use this trick like SQLAlchemy, Pytantic, etc.

@amirsoroush
Copy link
Contributor Author

@amirsoroush And wdym by:

Also __getattr__ is a bit slow right now.

All the functions are static and right now its not more that 10-15 of them, why would it be slow? even if we increase the iteration to 100-500 it will work just fine am i missing sth here?

Suppose you want to use zadd() which is on the last strategy, These are the steps needed for it to be found on every single call:

  1. A normal attribute lookup (object.__getattribute__) fails to find .zadd.
  2. The custom __getattr__ is executed.
  3. It loops through self.strategies to find that there a strategy which has that name.
  4. It creates a new function object and returns it.
  5. The function itself loops through the self.strategies once again to find the attribute and call it.

Not a huge concern but it may make a difference in tight loops.

@hamidrabedi
Copy link
Owner

There are other modules that use this trick like SQLAlchemy, Pytantic, etc.

Ok looks good, can you add some sample to core file and maybe on one of the datatypes?

@hamidrabedi
Copy link
Owner

Not a huge concern but it may make a difference in tight loops.

We could save the related strategy (if existed) to the key and value then there is no need to iterate over strategies.
But need to think throw this one.
I will add some performance tests to see the cost of iterating over all funcs and strats.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants