You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
As an (temporary?) alternative to full support for Python typing (#69), I'd like to propose adding multipledispatch.TypedList. The use case here is separate functions for different list subtypes, e.g., lists of strings vs lists of integers. See pydata/xarray#1938 for discussion.
The exact public facing API is up for discussion. I'm tentatively calling this TypedList for clarity and to distinguish this from typing.List (this is actually equivalent to typing.List[typing.Union[...]).
I don't like that this merges the idea of typed lists and regular lists. One of the advantages of the implementation in blaze is that it makes the two kinds of dispatches distinct. This is important when working on a large code base with thousands of dispatch definitions, many of them already dispatch on list. The proposed implementation adds ambiguity when you register a dispatch for both list and TypedList. For example, what should the result of the following code be?
Given that we can get the the same auto-boxing behavior with two lines of Python, I am not sure this is a big feature. If anything, it forces us to explicitly register the list instance so we know that this has been done and we understand how dispatching will happen in the future.
Regarding the use of frozenset vs unique: you are correct that the frozenset version will create less classes and probably be a bit faster, but due to how the subclassing machinery works it still produces the correct result.
Finally, the VarArgs object is duck-typed like an immutable list, so it should be usable in almost all cases without worrying about the actual type, this is Python after all.
I would also like to make my stance on typing clear: I don't think that supporting typing style annotations in multipledispatch is a good idea. typing annotations are designed for static analysis tools like mypy, they should not affect the runtime behavior of your program.
I'm also happy with a solution based more directly on @llllllllll's VarArgs. It is indeed simple to write a decorator to automatically unwrap dispatched types and to overload list directly, so I'm all for leaving that out of multipledispatch.
The harder part is writing the TypedList/VarArgs type in the first place, which involves some subtlety and metaclasses. Is there support for adding some form of this into multipledispatch?
As for typing, I'll note that there are at least two ways to support it:
Types from typing in the @dispatch decorator. This form is very explicit:
I would support version (1) directly in multipledispatch.dispatch if it can be done in a performant way. I'm not opposed to version (2), but it should probably use a separate name, e.g., dispatch_by_type_hints or something similar.
Note that multipledispatch does support PEP 484 type hints: #65
Oh, interesting.
I'm not entirely sure that's a good idea to include for the default dispatch decorator, to be honest :). Types for dispatching with multipledispatch and type checking (e.g., with mypy) have some small but important differences:
multipledispatch only accepts exact type matches (not even subclasses) for runtime performance (edit: not true)
type checkers will even introspect values to determine compound types (e.g., List[str]) and have various other small differences from runtime type checking (e.g., integers are valid for float arguments).
I can see cases where I might use different types, e.g., list for multipledispatch but List[str] for type checking.
Subtypes can come in useful. For example, I wouldn't feel okay asking XArray to change its code every time I add a new matrix format. And it will be useful in sparse for checking input arguments are a SparseArray instead of every combination.
In light of @llllllllll's comments, I'm now in favour of @llllllllll's implementation, maybe while adding a simple wrapper that extracts the List from VarArgs so that the function itself doesn't have to be written to extract all the arguments. And possibly a wrapper that converts particular arguments to VarArgs.
Hi everybody,
in one library that I am currently developing I had done something similar (typed lists), see e.g. the following tests for the notation
# Test class with list_of inputs
def test_enabled_list_of():
class E(object):
@dispatch(list_of(int))
def __init__(self, arg):
self.arg = arg[0]
assert E([1, 2]).arg == 1
# Test class with list_of inputs, with multiple types for each list
def test_enabled_list_of_multiple():
class E(object):
@dispatch(list_of(float))
def __init__(self, arg):
self.arg = arg[0]
@dispatch(list_of((float, int)))
def __init__(self, arg):
self.arg = 2*arg[0]
@dispatch(list_of((float, str)))
def __init__(self, arg):
self.arg = 3*arg[0]
assert E([1.]).arg == 1. and isinstance(E([1., 2.]).arg, float)
assert E(["a"]).arg == "aaa"
assert E([1]).arg == 2 and isinstance(E([1]).arg, int)
assert E([1., 2.]).arg == 1. and isinstance(E([1., 2.]).arg, float)
assert E([4., 3]).arg == 8. and isinstance(E([4., 3]).arg, float)
assert E([4, 3.]).arg == 8 and isinstance(E([4, 3.]).arg, int)
assert E([2., "a"]).arg == 6. and isinstance(E([2., "a"]).arg, float)
assert raises(UnavailableSignatureError, lambda: E([object()]))
# Test class with list_of inputs, with multiple types for each list, with ambiguous types
def test_enabled_list_of_multiple_ambiguous():
class E(object):
@dispatch(list_of(float))
def __init__(self, arg):
self.arg = arg[0]
@dispatch(list_of((float, int)))
def __init__(self, arg):
self.arg = 2*arg[0]
@dispatch(list_of((float, int, str)))
def __init__(self, arg):
self.arg = 3*arg[0]
try:
# Ambiguity checking is delayed to the first time the method is evaluated
E = E(None)
# The ambiguity for list_of(float) is solved by the first method, however
# the ambiguity for list_of(int) cannot be solved as both the second and third
# method could be used for that.
except AmbiguousSignatureError:
pass # this has failed has expected
else:
raise RuntimeError("This test should have failed due to list_of(int) ambiguity")
# Ambiguity can be fixed by explicitly providing an overloaded method for list_of(int)
class F(object):
@dispatch(list_of(float))
def __init__(self, arg):
self.arg = arg[0]
@dispatch(list_of(int))
def __init__(self, arg):
self.arg = 4*arg[0]
@dispatch(list_of((float, int)))
def __init__(self, arg):
self.arg = 2*arg[0]
@dispatch(list_of((float, int, str)))
def __init__(self, arg):
self.arg = 3*arg[0]
assert F([1.]).arg == 1. and isinstance(F([1., 2.]).arg, float)
assert F(["a"]).arg == "aaa"
assert F([1]).arg == 4 and isinstance(F([1]).arg, int)
assert F([1., 2.]).arg == 1. and isinstance(F([1., 2.]).arg, float)
assert F([4., 3]).arg == 8. and isinstance(F([4., 3]).arg, float)
assert F([4, 3.]).arg == 8 and isinstance(F([4, 3.]).arg, int)
assert F([2., "a"]).arg == 6. and isinstance(F([2., "a"]).arg, float)
# Test inheritance in combination with list_of
def test_inheritance_for_list_of():
class A(object):
pass
class B(object):
pass
class C(A):
pass
@dispatch(list_of(A))
def f(x):
return 'a'
@dispatch(list_of(B))
def f(x):
return 'b'
assert f([A(), A()]) == 'a'
assert f([B(), B()]) == 'b'
assert f([C(), C()]) == 'a'
assert f([C(), A()]) == 'a'
assert raises(UnavailableSignatureError, lambda: f(B(), C()))
Right now the code is a bit of hack on top of multipledispatch.
There is definitely some overlapping to PR 69 [*], but I can definitely share the code in a gist if you want to have a look [**]. Let me know.
[*] I created wrapper called list_of instead of using the typing module, which I thought was not available in python 2, and I felt it was closer to the existing multipledispatch notation to have list_of((type1, type2)) rather than List[Union[type1, type2]]. Even though the typing module is probably superior to my approach, maybe some of my test cases could still come useful.
[**] I am proposing a gist and not a PR because in any case the code would need a bit of cleanup
Finally, the VarArgs object is duck-typed like an immutable list, so it should be usable in almost all cases without worrying about the actual type, this is Python after all.
@llllllllll This is one part that I don't love about your solution, for two reasons:
Reliability: relying on duck-typing means VarArgs objects will be used with libraries that weren't built for it. As you note, this usually works, but there are many exceptions where base types (especially tuple) treated differently. Offhand, I can think of examples in NumPy, Dask and Xarray. Even if duck types currently work, relying on that behavior introduces risk of being broken by future releases, since upstream libraries rarely test for full duck-type compatibility.
Performance: Casting a list or tuple to VarArgs and then back to a base sequence type (if desired) entails additional overhead, because the the full sequence is copied for each conversion. For long sequences, this overhead could be non-negligible. I would rather only pay the price of iterating over a sequence once, to determine its unique types.
So again, I really don't care exactly how we spell this, but clearly there's a strong need. To keep things moving forward, let me make another proposal of what the public API could look like here:
frommultipledispatchimportdispatch, VarArgs@dispatch(VarArgs[int])deff(varargs):
print('integers:', varargs.value)
@dispatch(VarArgs[str])deff(varargs):
print('strings:', varargs.value)
@dispatch(list)deff(value):
returnf(VarArgs(value))
f([1, 2]) # integers: [1, 2]f(['foo', 'bar']) # strings: ['foo', 'bar']f([[1, 2]]) # NotImplementedError: Could not find signature for f: <VarArgs[list]>
My main goal with this version is to keep things explicit and maintainable. We'll leave syntactic sugar up to downstream libraries:
Explicit wrapping with VarArgs() is required to wrap a sequence (and infer the combined type)
Explicit unwrapping with varargs.value is required to pull out the wrapped sequence.
We only support a single type VarArgs for sequences of values with an inferred type. Downstream libraries can make their own specialized functions for different sequence types if desired.
We do need a metaclass to implement __getitem__ on the VarArgs type, but the usage is pretty tame as far as metaclasses go. This magic also has clear upsides: we need dynamically created classes to make this work at all, and this at least provides a clear baseclass and constructor for the specialized VarArgs types.
In xarray, ~10,000 elements is probably a reasonable upper bound on concat. Still, I agree that the overhead of converting to tuple is will be negligible for all the use-cases I can currently foresee.
So I'm OK always converting the args to VarArgs to a tuple. I would still rather that it uses composition to hold a tuple in varargs.value, and implement sequence API via forwarding methods rather than subclassing directly from tuple. Unnecessary subclassing makes me nervous.
+1 for composition
-1 for copying (it needs O(n) RAM as well as the the performance hit)
+1 for allowing mutable types and clearly documenting that mutations should preserve the types.
-1 for copying (it needs O(n) RAM as well as the the performance hit)
Personally I'm fine with copying. I don't think that this will have an impact on performance. @hameerabbasi you may be concerned about copying all of the arrays when passing a list of arrays. I agree that this would be bad. However I don't think that that is actually what is being discussed here. I believe that this would just cause a copy of the the container holding all of the references. I don't think that asymptotic arguments should necessarily hold sway here.
In [1]: importnumpyasnpIn [2]: x=np.arange(100000000)
In [3]: L= [x] *10000In [4]: %time_=tuple(L)
CPUtimes: user159µs, sys: 31µs, total: 190µsWalltime: 199µs
I may not be fully understanding the conversation here though.
I understand that fully (the part where you only copy references). 😅 I might be biased here, but I'm pretty sure that:
There are really no use-cases for mutating a collection with any types other than the initial ones.
It actually allows mutations on the original object (keeping the types), if needed, rather than blocking them outright.
I understand the lists themselves are likely to be very small (and the memory/performance overhead insignificant). But my OCD screams at me: "O(n) memory when it could be O(1)? Madness!", but feel free to ignore it. 😅
I understand the lists themselves are likely to be very small (and the memory/performance overhead insignificant). But my OCD screams at me: "O(n) memory when it could be O(1)? Madness!", but feel free to ignore it.
OK, good to know. Does that mean that you're removing your -1 above? I generally interpret -1 as "I am sufficiently against this idea that I block it from moving forward. I require that we find an alternative."
There are really no use-cases for mutating a collection with any types other than the initial ones.
It actually allows mutations on the original object, if needed, rather than blocking them outright.
OK, these might be more serious reasons. I'm not up-to-date on this topic enough to judge.
Yes, I remove the -1. Another reason I thought of: If you were to do it again to a mutated collection, it'll take another O(n) instead of using the cached value.
I am fine with having a tuple attribute instead of subclassing and implementing a sequence API that forwards to this tuple. This has the added benefit of not making the type partially orderable with tuple itself.
Allowing mutation will just cause hard to find bugs unless we wrap the mutable collection in some magic object that changes type based on the inserted elements, but then we are back to O(n) to check all the inserted elements so we might as well copy. I also know that some people dislike dynamically changing your __class__. The code I foresee is:
In the recursive call, the sequence will still be typed as [int], but now we just inserted a string. The actual insert site doesn't obviously look incorrect, nor does the call site, assuming Class() can be added to integers, but this code would do the wrong thing. If we copied into a new typed sequence box, then we would get something of type [int, Class] which would search for a new dispatch.
I would also like to make a side-note about the name: VarArgs: in blaze this was built to handle expression with variadic arguments, and the actual APIs that produce them are built from *args. If we generalized their use to arbitrary sequences, I think we could use another name like TypedSequence or TypedList or something, because out of context VarArgs seems a bit weird.
Wouldn't this sort of thing be fixed by making the decorator @dispatch(TypedSequence[int], Optional[int])? I get that someone could write code that does this, but is there a use-case for this?
multipledispatch doesn't kick in for keyword arguments. I think the function I wrote is a very real example, you could definite have an optimized fast path for foldl with all ints, vs foldl with arbitrary types.
Ah, fair enough. I'm on board with copying, but I'd rather we built a keyword argument feature into this library. Something like @dispatch(TypedSequence[int], starting_value=Optional[int]).
Activity
mrocklin commentedon Feb 26, 2018
cc @llllllllll @mariusvniekerk
hameerabbasi commentedon Feb 26, 2018
I think that for tuples, it might be useful to have
Tuple(exact match) andVarTuple(this approach). Tuples are often fixed length.Edit: Or, more generally,
IterableandVarIterable.llllllllll commentedon Feb 26, 2018
I don't like that this merges the idea of typed lists and regular lists. One of the advantages of the implementation in blaze is that it makes the two kinds of dispatches distinct. This is important when working on a large code base with thousands of dispatch definitions, many of them already dispatch on
list. The proposed implementation adds ambiguity when you register a dispatch for bothlistandTypedList. For example, what should the result of the following code be?This currently prints
regular; however, if you change the definition order of the two functions you will gettyped. I find this confusing given that:Given that we can get the the same auto-boxing behavior with two lines of Python, I am not sure this is a big feature. If anything, it forces us to explicitly register the
listinstance so we know that this has been done and we understand how dispatching will happen in the future.Regarding the use of
frozensetvsunique: you are correct that the frozenset version will create less classes and probably be a bit faster, but due to how the subclassing machinery works it still produces the correct result.Finally, the
VarArgsobject is duck-typed like an immutable list, so it should be usable in almost all cases without worrying about the actual type, this is Python after all.I would also like to make my stance on
typingclear: I don't think that supportingtypingstyle annotations in multipledispatch is a good idea.typingannotations are designed for static analysis tools like mypy, they should not affect the runtime behavior of your program.hameerabbasi commentedon Feb 27, 2018
I, on the other hand, support typing. But (on the other hand) Joe is right about overlapping, and how it could be difficult in a large codebase.
I suggest writing a simple decorator on anything that will convert
VarArgs/TypedListtoList. Should be trivial.shoyer commentedon Feb 28, 2018
I'm also happy with a solution based more directly on @llllllllll's
VarArgs. It is indeed simple to write a decorator to automatically unwrap dispatched types and to overloadlistdirectly, so I'm all for leaving that out ofmultipledispatch.The harder part is writing the
TypedList/VarArgstype in the first place, which involves some subtlety and metaclasses. Is there support for adding some form of this into multipledispatch?As for typing, I'll note that there are at least two ways to support it:
typingin the@dispatchdecorator. This form is very explicit:I would support version (1) directly in
multipledispatch.dispatchif it can be done in a performant way. I'm not opposed to version (2), but it should probably use a separate name, e.g.,dispatch_by_type_hintsor something similar.mrocklin commentedon Feb 28, 2018
Note that multipledispatch does support PEP 484 type hints: #65
shoyer commentedon Feb 28, 2018
Oh, interesting.
I'm not entirely sure that's a good idea to include for the default
dispatchdecorator, to be honest :). Types for dispatching with multipledispatch and type checking (e.g., with mypy) have some small but important differences:multipledispatch only accepts exact type matches (not even subclasses) for runtime performance(edit: not true)List[str]) and have various other small differences from runtime type checking (e.g., integers are valid forfloatarguments).I can see cases where I might use different types, e.g.,
listfor multipledispatch butList[str]for type checking.llllllllll commentedon Feb 28, 2018
Multiple dispatch supports subclasses correctly. It uses the same linearization algorithm as the Python MRO to partially order the types to search.
shoyer commentedon Feb 28, 2018
Oops, my mistake.
hameerabbasi commentedon Feb 28, 2018
Subtypes can come in useful. For example, I wouldn't feel okay asking XArray to change its code every time I add a new matrix format. And it will be useful in sparse for checking input arguments are a
SparseArrayinstead of every combination.In light of @llllllllll's comments, I'm now in favour of @llllllllll's implementation, maybe while adding a simple wrapper that extracts the
ListfromVarArgsso that the function itself doesn't have to be written to extract all the arguments. And possibly a wrapper that converts particular arguments toVarArgs.francesco-ballarin commentedon Mar 1, 2018
Hi everybody,
in one library that I am currently developing I had done something similar (typed lists), see e.g. the following tests for the notation
Right now the code is a bit of hack on top of multipledispatch.
There is definitely some overlapping to PR 69 [*], but I can definitely share the code in a gist if you want to have a look [**]. Let me know.
[*] I created wrapper called list_of instead of using the typing module, which I thought was not available in python 2, and I felt it was closer to the existing multipledispatch notation to have list_of((type1, type2)) rather than List[Union[type1, type2]]. Even though the typing module is probably superior to my approach, maybe some of my test cases could still come useful.
[**] I am proposing a gist and not a PR because in any case the code would need a bit of cleanup
shoyer commentedon Mar 4, 2018
@llllllllll This is one part that I don't love about your solution, for two reasons:
VarArgsobjects will be used with libraries that weren't built for it. As you note, this usually works, but there are many exceptions where base types (especiallytuple) treated differently. Offhand, I can think of examples in NumPy, Dask and Xarray. Even if duck types currently work, relying on that behavior introduces risk of being broken by future releases, since upstream libraries rarely test for full duck-type compatibility.VarArgsand then back to a base sequence type (if desired) entails additional overhead, because the the full sequence is copied for each conversion. For long sequences, this overhead could be non-negligible. I would rather only pay the price of iterating over a sequence once, to determine its unique types.So again, I really don't care exactly how we spell this, but clearly there's a strong need. To keep things moving forward, let me make another proposal of what the public API could look like here:
My main goal with this version is to keep things explicit and maintainable. We'll leave syntactic sugar up to downstream libraries:
VarArgs()is required to wrap a sequence (and infer the combined type)varargs.valueis required to pull out the wrapped sequence.VarArgsfor sequences of values with an inferred type. Downstream libraries can make their own specialized functions for different sequence types if desired.We do need a metaclass to implement
__getitem__on theVarArgstype, but the usage is pretty tame as far as metaclasses go. This magic also has clear upsides: we need dynamically created classes to make this work at all, and this at least provides a clear baseclass and constructor for the specializedVarArgstypes.shoyer commentedon Mar 4, 2018
We could easily make
VarArgsitself implement the immutable sequence API. But it shouldn't inherit fromtuple.hameerabbasi commentedon Mar 4, 2018
Don't you mean
f(VarArgs([1, 2]))?shoyer commentedon Mar 4, 2018
In this example, my function is explicitly unwrapping the original value with
varargs.value.6 remaining items
shoyer commentedon Apr 16, 2018
In xarray, ~10,000 elements is probably a reasonable upper bound on
concat. Still, I agree that the overhead of converting to tuple is will be negligible for all the use-cases I can currently foresee.So I'm OK always converting the args to
VarArgsto a tuple. I would still rather that it uses composition to hold a tuple invarargs.value, and implement sequence API via forwarding methods rather than subclassing directly from tuple. Unnecessary subclassing makes me nervous.hameerabbasi commentedon Apr 17, 2018
+1 for composition
-1 for copying (it needs O(n) RAM as well as the the performance hit)
+1 for allowing mutable types and clearly documenting that mutations should preserve the types.
mrocklin commentedon Apr 19, 2018
Personally I'm fine with copying. I don't think that this will have an impact on performance. @hameerabbasi you may be concerned about copying all of the arrays when passing a list of arrays. I agree that this would be bad. However I don't think that that is actually what is being discussed here. I believe that this would just cause a copy of the the container holding all of the references. I don't think that asymptotic arguments should necessarily hold sway here.
I may not be fully understanding the conversation here though.
hameerabbasi commentedon Apr 19, 2018
I understand that fully (the part where you only copy references). 😅 I might be biased here, but I'm pretty sure that:
I understand the lists themselves are likely to be very small (and the memory/performance overhead insignificant). But my OCD screams at me: "
O(n)memory when it could beO(1)? Madness!", but feel free to ignore it. 😅mrocklin commentedon Apr 19, 2018
OK, good to know. Does that mean that you're removing your -1 above? I generally interpret -1 as "I am sufficiently against this idea that I block it from moving forward. I require that we find an alternative."
OK, these might be more serious reasons. I'm not up-to-date on this topic enough to judge.
hameerabbasi commentedon Apr 19, 2018
Yes, I remove the -1. Another reason I thought of: If you were to do it again to a mutated collection, it'll take another
O(n)instead of using the cached value.llllllllll commentedon Apr 19, 2018
I am fine with having a tuple attribute instead of subclassing and implementing a sequence API that forwards to this tuple. This has the added benefit of not making the type partially orderable with tuple itself.
Allowing mutation will just cause hard to find bugs unless we wrap the mutable collection in some magic object that changes type based on the inserted elements, but then we are back to O(n) to check all the inserted elements so we might as well copy. I also know that some people dislike dynamically changing your
__class__. The code I foresee is:In the recursive call, the sequence will still be typed as
[int], but now we just inserted a string. The actual insert site doesn't obviously look incorrect, nor does the call site, assumingClass()can be added to integers, but this code would do the wrong thing. If we copied into a new typed sequence box, then we would get something of type[int, Class]which would search for a new dispatch.I would also like to make a side-note about the name:
VarArgs: in blaze this was built to handle expression with variadic arguments, and the actual APIs that produce them are built from*args. If we generalized their use to arbitrary sequences, I think we could use another name likeTypedSequenceorTypedListor something, because out of contextVarArgsseems a bit weird.hameerabbasi commentedon Apr 19, 2018
Wouldn't this sort of thing be fixed by making the decorator
@dispatch(TypedSequence[int], Optional[int])? I get that someone could write code that does this, but is there a use-case for this?llllllllll commentedon Apr 19, 2018
multipledispatch doesn't kick in for keyword arguments. I think the function I wrote is a very real example, you could definite have an optimized fast path for
foldlwith all ints, vsfoldlwith arbitrary types.hameerabbasi commentedon Apr 19, 2018
Ah, fair enough. I'm on board with copying, but I'd rather we built a keyword argument feature into this library. Something like
@dispatch(TypedSequence[int], starting_value=Optional[int]).llllllllll commentedon Apr 19, 2018
I agree that it would be useful to support keyword arguments, but that should be discussed in a new issue.
shoyer commentedon Apr 19, 2018
hameerabbasi commentedon Apr 24, 2018
+1 for TypedSequence and getting this in as quickly as possible. 😅
shoyer commentedon Apr 24, 2018
fix: rebuild_aggregate function signature