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

Precedence issue introduced by redefinition of method in inheritor class #130

Open
femtomc opened this issue Jan 24, 2024 · 13 comments
Open

Comments

@femtomc
Copy link

femtomc commented Jan 24, 2024

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?

@PhilipVinc
Copy link
Collaborator

can you share a MWE?

@femtomc
Copy link
Author

femtomc commented Jan 24, 2024

Sure! will prepare one and send this afternoon

@femtomc
Copy link
Author

femtomc commented Jan 24, 2024

@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())

@wesselb
Copy link
Member

wesselb commented Jan 25, 2024

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

@femtomc
Copy link
Author

femtomc commented Jan 25, 2024

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
Copy link
Author

femtomc commented Jan 25, 2024

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

@wesselb
Copy link
Member

wesselb commented Jan 26, 2024

@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

@PhilipVinc
Copy link
Collaborator

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...

@femtomc
Copy link
Author

femtomc commented Jan 26, 2024

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
Copy link
Author

femtomc commented Jan 26, 2024

@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
Copy link
Author

femtomc commented Jan 26, 2024

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

@femtomc
Copy link
Author

femtomc commented Jan 26, 2024

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)

@wesselb
Copy link
Member

wesselb commented Feb 11, 2024

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.

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

No branches or pull requests

3 participants