Plum trying to double-register in doctest/pytest

[nstarman]

The issue seems to be that plum is trying to register the same function twice! E.g. I'm seeing this and both impl have the exact same memory address.

plum.resolver.MethodRedefinitionWarning: `Method(function_name='arange', signature=Signature(typing.Union[ArrayLike], typing.Optional[ArrayLike], typing.Optional[A, return_type=typing.Union[ArrayLike], impl=<function arange at 0x7feadc790af0>)` overwrites the earlier definition `Method(function_name='arange', signature=Signature(typing.Union[ArrayLike], typing.Optional[ArrayLike], typing.Optional[A, return_type=typing.Union[ArrayLike], impl=<function arange at 0x7feadc790af0>)`.

Originally posted by @nstarman in #166 (comment)

[wesselb]

@nstarman Do you have a MWE? Or could you link to a CI run which exhibits the warning when it shouldn't?

I've been investigating redefinition warnings on my end, and have already uncovered a few issues in both Plum and packages using Plum. This seems to have been a very useful addition!

[nstarman]

https://github.com/GalacticDynamics/quaxed/actions/runs/9576983536/job/26404429851 for a CI run. Without line numbers I wasn't able to trace back to confirm.
What's the motivation for delaying the MethodRedefinitionWarning?

[wesselb]

Thanks, @nstarman! The methods are registered upon the first function invocation, not when these methods are defined, to deal with types that might not yet exist when the function is defined, e.g. in the case of forward references.

With regard to your redefinition warnings, I had a look at the internals of quaxed, and I think I've identified where they come from. In summary, the redefinitions arise from the way that Plum handles non-keyword-only arguments with default values. I think this is best illustrated with an example:

from numbers import Number
from typing import Optional

import numpy as np

from plum import dispatch


@dispatch
def arange1(
    start: Number,
    /,
    stop: Optional[Number] = None,
    step: Optional[Number] = None,
):
    return np.arange(start, stop)

print(arange1.methods)

@dispatch
def arange2(
    start: Number,
    stop: Optional[Number] = None,
    *,
    step: Optional[Number] = None,
):
    return np.arange(start, stop)

print(arange2.methods)

This gives the following output:

List of 3 method(s):
    [0] arange1(start: numbers.Number, stop: typing.Optional[numbers.Number], step: typing.Optional[numbers.Number])
        <function arange1 at 0x7f96f06be280> @ /<ipython-input-1-7a6722e6860e>:9
    [1] arange1(start: numbers.Number, stop: typing.Optional[numbers.Number])
        <function arange1 at 0x7f96f06be280> @ /<ipython-input-1-7a6722e6860e>:9
    [2] arange1(start: numbers.Number)
        <function arange1 at 0x7f96f06be280> @ /<ipython-input-1-7a6722e6860e>:9

List of 2 method(s):
    [0] arange2(start: numbers.Number, stop: typing.Optional[numbers.Number], *, step)
        <function arange2 at 0x7f96901d9a60> @ /<ipython-input-1-7a6722e6860e>:20
    [1] arange2(start: numbers.Number, *, step)
        <function arange2 at 0x7f96901d9a60> @ /<ipython-input-1-7a6722e6860e>:20

Note that the last two methods in each list have the same signature (keyword arguments don't count). These are exactly which are redefined:

from numbers import Number
from typing import Optional

import numpy as np

from plum import dispatch


@dispatch
def arange(
    start: Number,
    /,
    stop: Optional[Number] = None,
    step: Optional[Number] = None,
):
    return np.arange(start, stop)

    
@dispatch
def arange(
    start: Number,
    stop: Optional[Number] = None,
    *,
    step: Optional[Number] = None,
):
    return np.arange(start, stop)


print(arange.methods)

MethodRedefinitionWarning: `Method(function_name='arange', signature=Signature(numbers.Number, typing.Optional[numbers.Number]), return_type=typing.Any, implementation=<function arange at 0x7fcf18763a60>)` (`/<ipython-input-1-b2c2436e1d91>:19`) overwrites the earlier definition `Method(function_name='arange', signature=Signature(numbers.Number, typing.Optional[numbers.Number]), return_type=typing.Any, implementation=<function arange at 0x7fcf38474820>)` (`/<ipython-input-1-b2c2436e1d91>:9`).
  warnings.warn(
MethodRedefinitionWarning: `Method(function_name='arange', signature=Signature(numbers.Number), return_type=typing.Any, implementation=<function arange at 0x7fcf18763a60>)` (`/<ipython-input-1-b2c2436e1d91>:19`) overwrites the earlier definition `Method(function_name='arange', signature=Signature(numbers.Number), return_type=typing.Any, implementation=<function arange at 0x7fcf38474820>)` (`/<ipython-input-1-b2c2436e1d91>:9`).
  warnings.warn(
List of 3 method(s):
    [0] arange(start: numbers.Number, stop: typing.Optional[numbers.Number], step: typing.Optional[numbers.Number])
        <function arange at 0x7fcf38474820> @ /<ipython-input-1-b2c2436e1d91>:9
    [1] arange(start: numbers.Number, stop: typing.Optional[numbers.Number], *, step)
        <function arange at 0x7fcf18763a60> @ /<ipython-input-1-b2c2436e1d91>:19
    [2] arange(start: numbers.Number, *, step)
        <function arange at 0x7fcf18763a60> @ /<ipython-input-1-b2c2436e1d91>:19

[nstarman]

Thanks for the diagnosis!
I have these double registrations on purpose, since dispatch doesn't look at keyword-only arguments.
But I see that the inclusion of defaults make these two examples identical.
I guess the solution is to get rid of the default values, right?

[nstarman]

Thanks! This solves most issues. I'll open a new issue for what it doesn't.

[wesselb]

@nstarman, perfect! As @PhilipVinc suggested, maybe the redefinition warnings are too aggressive and we should provide a mechanism to silence them. I suggested an opt out mechanism for packages, but perhaps making it an option of Dispatcher is a better way to go about it.

[PhilipVinc]

I had two double definitions so this helped me notice them.
Still in the future I think it's handier to give a grace period for package owners to adapt to changes that can affect end users...

[wesselb]

You’re right that this was too big of a change at once.

Even though it is turning out to be a useful debug tool, I’m thinking that throwing a warning at every redefinition is too aggressive, as there could be scenarios where redefinitions are intended.

I’m considering to make redefinition warnings opt in, in the following way: dispatch = Dispatcher(warn_redefinition=True). How would that look to you?