i2mint / dol

Data Object Layer
MIT License
4 stars 2 forks source link

`dol.base.Store.wrap` breaks unbound method calls. #17

Open andeaseme opened 1 year ago

andeaseme commented 1 year ago

dol.base.Store.wrap breaks unbound method calls.

What's an "unbound method call"? See this Essentially, when you call Klass.method(...) (whether method is a normal, class, or static)

See more of the context and easier to test code here.

_Note: The original issue was entitled "Adding wrap_kvs...", but changed this because the root cause is dol.base.Store.wrap, not wrap_kvs itself._

Test code:

from dol.base import Store

def test_wrap_kvs_vs_class_and_static_methods():
    """Adding wrap_kvs breaks methods when called from class

    That is, when you call Klass.method() (where method is a normal, class, or static)

    https://github.com/i2mint/dol/issues/17
    """

    @Store.wrap
    class MyFiles:
        y = 2

        def normal_method(self, x=3):
            return self.y * x

        @classmethod
        def hello(cls):
            print('hello')

        @staticmethod
        def hi():
            print('hi')

    errors = []

    # This works fine!
    instance = MyFiles()
    assert instance.normal_method() == 6

    # But calling the method as a class...
    try:
        MyFiles.normal_method(instance)
    except Exception as e:
        print('method normal_method is broken by wrap_kvs decorator')
        print(f'{type(e).__name__}: {e}')
        errors.append(e)

    try:
        MyFiles.hello()
    except Exception as e:
        print('classmethod hello is broken by wrap_kvs decorator')
        print(f'{type(e).__name__}: {e}')
        errors.append(e)

    try:
        MyFiles.hi()
    except Exception as e:
        print('staticmethod hi is broken by wrap_kvs decorator')
        print(f'{type(e).__name__}: {e}')
        errors.append(e)

    if errors:
        first_error, *_ = errors
        raise first_error

# launch test:
test_wrap_kvs_vs_class_and_static_methods()

Output:

method normal_method is broken by wrap_kvs decorator
TypeError: 'DelegatedAttribute' object is not callable
classmethod hello is broken by wrap_kvs decorator
TypeError: 'DelegatedAttribute' object is not callable
staticmethod hi is broken by wrap_kvs decorator
TypeError: 'DelegatedAttribute' object is not callable
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In [28], line 55
     51         first_error, *_ = errors
     52         raise first_error
---> 55 test_wrap_kvs_vs_class_and_static_methods()

Cell In [28], line 52, in test_wrap_kvs_vs_class_and_static_methods()
     50 if errors:
     51     first_error, *_ = errors
---> 52     raise first_error

Cell In [28], line 30, in test_wrap_kvs_vs_class_and_static_methods()
     27 errors = []
     29 try:
---> 30     MyFiles.normal_method()
     31 except Exception as e:
     32     print('method normal_method is broken by wrap_kvs decorator')

TypeError: 'DelegatedAttribute' object is not callable
thorwhalen commented 1 year ago

Thanks @andeaseme -- will look into it. What python version do you use?

thorwhalen commented 1 year ago

I ask, because I wonder if it has to do with this 3.10... "bug".

There, you'll see, that it's functools.wraps and staticmethod that suddenly stopped working together.

thorwhalen commented 1 year ago

Nope -- apparently it also doesn't work in 3.8 (pushed your test to dol and CI validation (only 3.8 at this point) breaks: https://github.com/i2mint/dol/actions/runs/4063630336/jobs/6996091544#step:7:280

thorwhalen commented 1 year ago

In fact, it has nothing to do with staticmethod and classmethod it seems. Even if you stick a normal method in there you stick in there will lead to the same error.

thorwhalen commented 1 year ago

Updated issue name and content, as well as dol test, to reflect this.

thorwhalen commented 1 year ago

To debug, follow the vine down this line of dol.base.Store:

    wrap = classmethod(partial(delegator_wrap, delegation_attr='store'))
thorwhalen commented 1 year ago

Easier (and closer to the problem) test code, explain more of the context.

What does Store.wrap do? It wraps classes or instances in such a way that mapping methods (like __iter__, __getitem__, __setitem__, __delitem__, etc.) are intercepted and transformed, but other methods are not (they stay as they were).

This test is about the "stay as they were" part, so let's start with a simple class that has a method that we want to keep untouched.

class K:
    def pass_through(self):
        return 'hi'
    def pass_through(self):
        return 'hi'
        return 'hi'

wrapping an instance

instance_of_k = K()
assert instance_of_k.pass_through() == 'hi'
wrapped_instance_of_k = Store.wrap(instance_of_k)
assert wrapped_instance_of_k.pass_through() == 'hi'

wrapping a class

WrappedK = Store.wrap(K)

instance_of_wrapped_k = WrappedK()
assert instance_of_wrapped_k.pass_through() == 'hi'

Everything seems fine, but the problem creeps up when we try to use these methods

through an "unbound call".

This is when you call the method from a class, feeding an instance.

With the original class, this works:

assert K.pass_through(K()) == 'hi'

But this gives us an error on the wrapped class

assert WrappedK.pass_through(WrappedK()) == 'hi'  # error

or even this:

assert WrappedK.pass_through(K()) == 'hi'  # error