Closed SylvainCorlay closed 8 years ago
@sccolbert Sure, but keyword arguments are the same runtime complexity as positional ones. There might be a slight cost to matching them with keywords during the function call. If you really care about performance, then the future of fast Python is probably PyPy and we don't know how costly keyword arguments will really be.
The reason cooperative multiple inheritance can't use positional arguments is because you can't be sure which parent you're calling (or if you're even calling one of your parents) when you call super, and so you can't know in what order to send your positional arguments. You have to pass by keyword arguments and let the parents claim the arguments they each want.
Sure, but keyword arguments are the same runtime complexity as positional ones.
This is not true. Calling a function with keyword arguments takes a different non-optimized pass through the interpreter. There are several different bytecodes for calling a function: https://github.com/python/cpython/blob/master/Python/ceval.c#L3174 https://github.com/python/cpython/blob/master/Python/ceval.c#L3190 https://github.com/python/cpython/blob/master/Python/ceval.c#L4710 // fast path (note the comment) https://github.com/python/cpython/blob/master/Python/ceval.c#L4891 // slow path
In [12]: def foo(a): pass
In [13]: def bar(**kw): pass
In [14]: d = {'a': None}
In [15]: %timeit foo(**d)
10000000 loops, best of 3: 141 ns per loop
In [16]: %timeit foo(d)
10000000 loops, best of 3: 75.2 ns per loop
In [17]: %timeit bar(**d)
10000000 loops, best of 3: 174 ns per loop
The reason cooperative multiple inheritance can't use positional arguments is because you can't be sure which parent you're calling
I would argue that using multiple inheritance with incompatible base classes is an anti-pattern.
I also think that using a single positional dict argument is more scalable in the long run. The dict can grow to as many keys as is necessary, and no one has to change their function signatures.
Because someone will do this:
In [20]: def baz(a, b): pass
In [21]: args = {'a': 1, 'b': 2}
In [22]: baz(**args)
Which will break as soon as the dict grows another key:
In [23]: args['c'] = 3
In [24]: baz(**args)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-24-a74867fdfb0d> in <module>()
----> 1 baz(**args)
TypeError: baz() got an unexpected keyword argument 'c'
@sccolbert I said it was the same runtime complexity. Both are constant average time per argument per function call. Runtime complexity defines the asymptotic growth in the number of operations. By the way, I'm pretty familiar with the code from recently implementing most of PEP 448 :)
Anyway, multiple inheritance is not an "anti-pattern" in Python. The pattern looks like this:
class Base:
def f(self, *, a, b):
pass
# do something with a, b and act as base class
class DerivedA(Base):
def f(self, *, c, **kwargs):
super().f(**kwargs)
# do something with c
class DerivedB(Base):
def f(self, *, d, **kwargs):
super().f(**kwargs)
# do something with d
class DerivedC(DerivedA, DerivedB):
def f(self, **kwargs):
super().f(c=2, d=5, **kwargs)
# etc.
Note how there's no problem with derived classes adding keyword arguments to the signature. Positional arguments don't have this benefit.
I said it was the same runtime complexity
Okay, if you want to be that technical, it's still not a true statement.
This function call has O(1)
cost for the arguments as they are passed on the stack:
foo(a, b, c)
And this function call has O(n)
cost for the arguments because a new dict is allocated and all args are copied into it:
foo(a=1, b=2, **rest)
Anyway, multiple inheritance is not an "anti-pattern" in Python
I didn't say that. I said that multiply inheriting from incompatible base classes is an anti-pattern. And it's not that it won't work, it's that it's (IMO) brittle and difficult to follow.
@sccolbert They're both O(1) per argument. The positional arguments are also individually unpacked and copied to the stack.
Well, I don't think it's brittle and you have no choice if you're going to do multiple inheritance.
The positional arguments are also individually unpacked and copied to the stack.
No, they aren't. Please see the link to the fast_function
"fast path" I linked above. The whole point of fast_function
is to short-circuit the pack/unpack cycle of arguments.
@sccolbert ah, I don't have time right now, but I believe you. Good to know.
However, I think that making it possible to multiply inherit from classes by using keyword arguments is more important than the runtime cost savings of using positional arguments.
However, I think that making it possible to multiply inherit from classes by using keyword arguments is more important than the runtime cost savings of using positional arguments.
My concern with that approach, is that it forces all of those base classes to accept **kw
as the last param, or they risk breaking if the contents of the dict ever changes. Though, for the particular case under discussion (change handlers), I don't think multiple-inheritance will actually be used much.
@sccolbert I'm not sure how much it will be used, but I'm definitely planning on inheriting from trait types. The base classes actually don't have to accept **kwargs
as you can see in my example. Only derived classes need to hoover them up so that they can forward them in their call super.
In your example, the base class will fail if it gets a keyword arg it doesn't expect:
dc = DerivedC()
dc.f(**{'k': 12})
Traceback (most recent call last):
File "foo.py", line 21, in <module>
dc.f(**{'k': 12})
File "foo.py", line 18, in f
super().f(c=2, d=5, **kwargs)
File "foo.py", line 8, in f
super().f(**kwargs)
File "foo.py", line 13, in f
super().f(**kwargs)
TypeError: f() got an unexpected keyword argument 'k'
This situation will occur if we pass the change args to handlers as **kw
and decide to add a key to that dict at some point in the future.
@sccolbert You want the base class method to fail if it gets unexpected arguments just like you would want any method to fail in that case. It's the derived class methods that shouldn't fail for arguments they don't know how to handle — it may be the case that a super class method does handle that argument, and it should get the chance.
Yes, I agree with that premise, but that's not the pattern we want here. We want the flexibility to pass an argument that contains variable state to any handler. That allows us to add more information to the change arg in the future, without breaking any existing handler functions.
It seems that we are near completion in terms of implementation of the proposals of this issue.
All these items seem to have been completed.
Great, let's try to put together a beta soon.
:+1: for a beta whenever someone with the rights finds the time.
Since Matplotlib's folks are starting to write traitlet-based APIs, we probably need to think of a roadmap for the future of the library if it is to be more widely adopted by the Scipy community.
There are some improvements that we could easily make without splitting traitlets in two different repos:
1. Deprecate trait attribute declaration with TraitType types instead of TraitType instances
(Implemented in #51 and #55 - merged)
2. Like in Atom, separate the metadata from the keyword arguments in TraitType's constructor.
(Implemented in #53 - merged)
3. A replacement for the cumbersome
on_trait_change
in the future, with a more convenient signature and a simpler name.observe
/unobserve
method instead of using aremove=True/False
argument withon_trait_change
.names
andtype
.The callbacks take a single
change
dictionary argument, containing@observe
decorator to register methods as trait change handlers.(Implemented in #61 - merged)
4. Deprecate the custom cross-validation magic methods
_*bar*_validate
to the benefit of a@validate('bar')
decorator.(Implemented in #73 - merged)
5. Since the base trait type now inherits from
BaseDescriptor
and other descriptors are defined to work well withHasTraits
, we could make the following changes:MetaHasTraits
intoMetaHasDescriptors
and deprecate the old name.HasTraits
calledHasDescriptors
.(Implemented in #70 - merged)
6. Deprecate the default-value initializer magic methods
_*bar*_default
to the benefit of a@default('bar')
decorator.(Implemented in #114 - merged)
7. What is the best place for a repository of extra trait types for common types in the scipy stack such as ` numpy arrays, pandas/xray data structures, and their (binary) serialization functions for widgets (or other clients of comms) and ipyparallel?
It would make sense to propose a reference implementation of those, otherwise we will see multiple competing implementation emerge in different projects.
Besides, it is unlikely that such things would be accepted in core Pandas and numpy as of now...
(Scipy Trait Types Incubator Proposal)
8. Would it make sense to have a
once
version ofon_trait_change
(nowobserve
)?(There seems to be mixed opinions on this. Deferred.)
9. A common pattern when observing an object is the following:
maybe we could facilitate this by adding a boolean argument to
observe
, stating whether to also run the callback right-away or not.This would especially be useful when registering observer with the decorator syntax.
10. One thing that we had in mind in the long-run for widgets is having finer-grained events for containers, such as
List
andDict
. The ability to create other descriptors than trait types, possibly outside of the traitlets repository could enable experiments in this directions, like an integration of @jdfreder 's eventful dicts and lists.One idea that could be in the scope of traitlets though is to add an attribute to the
change
dictionary indicating the type of notification that is being sent.The last attribute could be used to define notification types corresponding to operational transforms.
Then, the
@observe
decorator would then have a 'type' keyword argument, (defaulting toNone
), to filter by notification type.The only thing to do to enable this possibility would be to add a
type
item in the dictionary and have the current implementation of observe filter on the notification type.(Implemented in #83 - merged)