plum icon indicating copy to clipboard operation
plum copied to clipboard

Precedence issue introduced by redefinition of method in inheritor class

Open femtomc opened this issue 1 year ago • 13 comments

Hi! I'm working on a design pattern which is a bit complicated, and I'm not sure how to proceed.

image

I've attached a screenshot which shows a pattern of dispatch I'm using. I have an abstract base class which defines a "NotImplemented" dispatch base for this method update (top of image), then I implement a specialized version with type dispatch on the type Mask -- now, Mask inherits from Choice.

Later on, I use the abstract base class and implement a subclass called SwitchCombinator. There, I redefine the method for the type Choice (but not the dispatch for Mask)!

Now, unfortunately, this appears to clobber the dispatch -- because when I test, even if I pass in a Mask -- the dispatch branch is the Choice one.

Is there a pattern which I can use here? Does this seem like something which should be supported?

femtomc avatar Jan 24 '24 15:01 femtomc

can you share a MWE?

PhilipVinc avatar Jan 24 '24 16:01 PhilipVinc

Sure! will prepare one and send this afternoon

femtomc avatar Jan 24 '24 16:01 femtomc

@PhilipVinc Here's an MWE:

from plum import dispatch

class Parent:
    pass

class B(Parent):
    pass

class BaseClass:
    # Specialized to B, which inherits from Parent.
    @dispatch
    def foo(self, x: B):
        print("hello!")
        
    # Generic -- not defined yet!
    @dispatch
    def foo(self, x: Parent):
        raise NotImplementedError
        
class Inheritor(BaseClass):
    # Inheritor of the BaseClass, okay I define a fallback
    # on the generic Parent base class.
    @dispatch
    def foo(self, x: Parent):
        print("cool!")
        
# Yay, specialized!
BaseClass().foo(B())

# Oops, clobbered.
Inheritor().foo(B())

femtomc avatar Jan 24 '24 20:01 femtomc

Hey @femtomc! It's not entirely clear to me which behaviour you would want. Should Inheritor().foo(B()) print something other than cool!?

It might be helpful to consider the following version, which gives an ambiguity error:

from plum import dispatch

class Parent:
    pass

class B(Parent):
    pass

class BaseClass:
    pass
        
class Inheritor(BaseClass):
    pass

@dispatch
def foo(self: BaseClass, x: Parent):
    raise NotImplementedError

@dispatch
def foo(self: BaseClass, x: B):
    print("hello!")

@dispatch
def foo(self: Inheritor, x: Parent):
    print("cool!")
        
foo(BaseClass(), B()) 

foo(Inheritor(), B())
# AmbiguousLookupError: `foo(<__main__.Inheritor object at 0x7fbbc85661f0>, <__main__.B object at 0x7fbbc8566220>)` is ambiguous.
#
# Candidates:
#    foo(self: __main__.BaseClass, x: __main__.B)
#        <function foo at 0x7fbbc8548820> @ /<ipython-input-2-859b87ba2416>:19
#    foo(self: __main__.Inheritor, x: __main__.Parent)
#        <function foo at 0x7fbba82acdc0> @ /<ipython-input-2-859b87ba2416>:23

wesselb avatar Jan 25 '24 11:01 wesselb

Right, so the thing I want is that Inheritor().foo(B()) prints "hello!" -- e.g. that switching the method on the parent class in the Inheritor would not affect the behavior of the specialization (that when I dispatch on B, I still get the original "hello!" behavior).

femtomc avatar Jan 25 '24 13:01 femtomc

The point being that, in Inheritor -- I could define a different fallback on the parent class Parent while preserving specialized behavior which I inherit from BaseClass

femtomc avatar Jan 25 '24 13:01 femtomc

@femtomc, how would something like this look?

from plum import dispatch

class Parent:
    pass

class B(Parent):
    pass

class BaseClass:
    pass
        
class Inheritor(BaseClass):
    pass

@dispatch
def foo_fallback(self: BaseClass, x: Parent):
    print("Fallback for BaseClass")

@dispatch
def foo_fallback(self: Inheritor, x: Parent):
    print("Fallback for Inheritor")

@dispatch
def foo(self: BaseClass, x: Parent):
    # If no specialisation is possible, run the fallback.
    return foo_fallback(self, x)

@dispatch
def foo(self: BaseClass, x: B):
    print("General specialisation")

        
foo(BaseClass(), Parent())  # Fallback for BaseClass
foo(BaseClass(), B())       # General specialisation

foo(Inheritor(), Parent())  # Fallback for Inheritor
foo(Inheritor(), B())       # General specialisation

wesselb avatar Jan 26 '24 08:01 wesselb

I think what you are asking is to treat the first argument as 'special' and always as maximally specialised for all sub-classes. Something along the lines of

class BaseClass:
    # Specialized to B, which inherits from Parent.
    @dispatch
    def foo(self, x: B): # implicitly stored as   foo(self: BaseClass, x: B)
        print("hello!")
        
    # Generic -- not defined yet!
    @dispatch
    def foo(self, x: Parent): # implicitly stored as   foo(self: BaseClass, x: Parent)
        raise NotImplementedError
        
class Inheritor(BaseClass):
    # implicitly defined
    @dispatch
    def foo(self: Inheritor, x: B):
        print("hello!")    
    @dispatch
    def foo(self: Inheritor, x: Parent):
        raise NotImplementedError

    # okay I define a fallback
    # on the generic Parent base class.
    @dispatch
    def foo(self, x: Parent):
        print("cool!")
        

where the implicit definitions are for all methods defined on a base class.

This model is probably something like single dispatch on the first argument and multiple dispatch on the others, therefore I agree that it seems to make sense in Python. But it is also somewhat different from just doing multiple dispatch on all arguments.

I'm unsure if this is the good way forward. However what I recognise is that we need some better tooling/way to work with MD in Python and marry it with existing paradigms such as OOP...

PhilipVinc avatar Jan 26 '24 13:01 PhilipVinc

Interesting, in my classes -- I did not annotate self. Does this affect dispatch in this weird combination of OOP and MD?

I can easily add annotations for self. Of course, I'll have to use "BaseClass" and "Inheritor", and I don't really know how the string annotations get resolved.

femtomc avatar Jan 26 '24 13:01 femtomc

@PhilipVinc I wonder if it's useful to have two dispatch patterns.

Multiple dispatch, as accessed via dispatch -- does the thing you'd expect, and behaves well with functions, etc.

But this sort of hybrid dispatch (maybe class_dispatch?) supports this hybrid style of OOP + MD? It has slightly different dispatch rules on the self argument.

It occurs to me that this is sort of like copying the dispatch table from the parent class to the child class, which can extend it or overwrite it as they see fit, while preserving fallbacks.

Edit: P.S. -- I'm not super familiar with this codebase, but if you gave me some tips I could try and hack up a version of this pattern and see how it works.

femtomc avatar Jan 26 '24 17:01 femtomc

I actually wonder if I can just hack this up by defining a new abstract base class, which owns a Dispatcher...

femtomc avatar Jan 26 '24 18:01 femtomc

Okay -- consider the following two screenshots.

If I don't define a new method foo in the inheritor class, I do inherit the method (and all its implementations) from the base class: image

However, if I do define foo -- it just completely replaces the method. image

So, in a sense, plum is already taking a stance on inherited methods -- the rule is: if you don't define a method with the same name, you'll inherit all the methods from the base class, otherwise -- you clobber the method table and remove all the methods from the base class.

I think my argument is that a (possibly more) useful stance is to merge the method tables. This is basically treating a base class like a "module" from Julia (for instance).

I'm aware that this will cause interesting scenarios: like, what happens if I inherit from two base classes that each define the same method? I think (just like in Julia), we should reject these scenarios as something which should cause an error (this is like telling the user: you can only define one fallback path). However, if one defines an inheritor with a successful merge, that inheritor can be used as a base class -- for further inheritance (I believe I'm essentially just describing the single dispatch pattern that @PhilipVinc described above, just with a bit more prose)

femtomc avatar Jan 26 '24 18:01 femtomc

So, in a sense, plum is already taking a stance on inherited methods -- the rule is: if you don't define a method with the same name, you'll inherit all the methods from the base class, otherwise -- you clobber the method table and remove all the methods from the base class.

Yes, you're right! When Plum was designed, we decided that we would handle inheritance in a way that would most naturally integrate with how Python handles inheritance: (1) Separate the method tables of class definitions. (2) When an appropriate method cannot be found, instead of raising a NotFoundLookupError, attempt the function of the parent according to Class.mro().

I agree that merging the method tables would be very interesting and could enable interesting new design patterns. It would, however, deviate from how method lookup in classes currently works (i.e. according to the MRO), which could be confusing. To decide whether this would be worth it, I guess one should figure out which new design patterns would be possible and whether those really aren't possible with the current model.

wesselb avatar Feb 11 '24 15:02 wesselb