Deprecate on_trait_change and magic change handlers

[SylvainCorlay]

Deprecation of on_trait_change and the signature of change handlers

As of now, this PR deprecates on_trait_change and the magic _*foo*_changed handlers, to the benefit of new observe and unobserve method. The signature of the handlers passed to observe is either

• no argument
• a single dict argument containing 'name', 'old', 'new' and 'owner', the HasTraits instance emitting the event.

See the examples below.

from __future__ import print_function
from traitlets import *

class Foo(HasTraits):
    bar = Int()

    def test_bar(self):
        print('foobar')

foo = Foo()    

def cb(change):
    print(change)

foo.observe(cb)
foo.bar = 3

Implementation of an observe decorator like in Atom:

On the other side, atom implements a observe decorator

class Person(Atom):
    age = Int()

    @observe('age')
    def debug_print(self, change):
        print(change['object'].last_name)

The problem is that the registration of the callback requires access self attribute, which is not available when the decorator is called. The way it works in atom is that the @observe('age') decorator replaces the debug_print method with a placeholder object ObserveHandler holding the method to be registered, which is to be picked up and unwrapped by the AtomMeta metaclass when instantiating the instance of (the counterpart to) HasTraits.

Rather than augmenting the metaclass, I could achieve the same result by making our ObserveHandler inherit from BaseDescriptor, and implement the custom logic in instance_init.

Example with traitlets:

class Foo(HasTraits):
    bar = Int()

    @observe('bar')
    def test(self):
        print('foobar')

foo = Foo()
foo.bar = 2

[SylvainCorlay]

PS: To handle the removal of the wrapped callbacks, I needed to write the wrapper to be comparable with the original function with __eq__.

[blink1073]

Missing space

[SylvainCorlay]

@ellisonbg @sccolbert as per earlier discussion.

[blink1073]

@SylvainCorlay, tested both constructs, 👍

[blink1073]

The old constructs worked too, but I didn't get any warnings when run in an IPython console.

[SylvainCorlay]

Hum, I am seeing the warnings in the notebook.

[blink1073]

For whatever reason my session is not printing DeprecationWarnings in the console or the notebook, I must have a global setting somewhere...

[jasongrout]

By default, deprecation warnings are not printed in python. You have to either manually set the warnings filter to allow them, or start python with a special switch.

[jasongrout]

Can we make this _CallbackWrapper? It feels more pythonic than using abbreviations.

[jasongrout]

Are the keys really optional?

[jasongrout]

@minrk, @ellisonbg - when you guys have a minute, it'd be great to hear your comments.

[SylvainCorlay]

Let me know what you think.

• While keeping the @observe decorator, we can rename the .observe method into .on_change.
• Another idea would be to remove the remove=False/True argument from the signature, and use a different method to unregister change hanldlers, like off_change.

[ellisonbg]

OK I see this one and will have a look...

On Sun, Aug 9, 2015 at 1:49 PM, Sylvain Corlay notifications@github.com
wrote:

Let me know what you think.

• While keeping the @observe decorator, we can rename the .observe
  method into .on_change.
• Another idea would be to remove the remove=False/True argument from
  the signature, and use a different method to unregister change hanldlers,
  like off_change.

—
Reply to this email directly or view it on GitHub
#61 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

[ellisonbg]

I think the name of the key in the change dict of object is too vague. What about source or parent?

[ellisonbg]

Good point!

On Thu, Aug 27, 2015 at 8:43 AM, Sylvain Corlay notifications@github.com
wrote:

Yes, it would make sense.

—
Reply to this email directly or view it on GitHub
#61 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

[SylvainCorlay]

So are we moving to 'emitter'?
I thought of it because I was looking at the ipywidget signaling PR: the state_changed signal sort of brings together widget attribute change notifications and signaling. It is one place where consistency in naming would be nice.

[ellisonbg]

Hmm, yeah, I can see how "emitter" has a "signal oriented" connotation
rather than the stateful model of traitlets which I think it better
described by "owner" (IOW, the "owner" owns that bit of state).

On Thu, Aug 27, 2015 at 8:49 AM, Sylvain Corlay notifications@github.com
wrote:

So are we moving to emitter?

—
Reply to this email directly or view it on GitHub
#61 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

[SylvainCorlay]

Although in the signaling PR, there are two bridges between traits and signaling:

• a widget attribute can be used as a slot (an Int() widget attribute can receive value from a signal having and Int() signature.)
• every widget has a global state_changed signal.

In the latter case especially, mixing two systems where one uses owner and the other one uses emitter may seem cumbersome.

[jasongrout]

+1 on emitter because it's more like source :)

[SylvainCorlay]

Pushed emitter instead of owner. Let's wait for @minrk to merge or ask to change back to 'owner' :)

[ellisonbg]

To clarify @minrk I think you are the right person to make this call. In
summary, there was a mild consensus around "owner" but "emitter" has
emerged as a late and popular option.

I am personally 50/50 on owner versus emitter.

On Thu, Aug 27, 2015 at 11:10 AM, Sylvain Corlay notifications@github.com
wrote:

Pushed emitter instead of owner. Let's wait for @minrk
https://github.com/minrk to merge or ask to change back to 'owner' :)

—
Reply to this email directly or view it on GitHub
#61 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

[rmorshea]

'source' sounded generic and ambiguous, 'emitter' seems better.

[minrk]

Owner also seems slightly preferable to emitter, but both seem just fine. I'm okay with letting the current state of this PR make the choice.

[ellisonbg]

We are getting @jdfreder to weigh in right now...

On Thu, Aug 27, 2015 at 11:54 AM, Min RK notifications@github.com wrote:

Owner also seems slightly preferable to emitter, but both seem just fine.
I'm okay with letting the current state of this PR make the choice.

—
Reply to this email directly or view it on GitHub
#61 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

[ellisonbg]

@jdfreder voted for emitter. I am still 50/50.

On Thu, Aug 27, 2015 at 11:58 AM, Brian Granger ellisonbg@gmail.com wrote:

We are getting @jdfreder to weigh in right now...

On Thu, Aug 27, 2015 at 11:54 AM, Min RK notifications@github.com wrote:

Owner also seems slightly preferable to emitter, but both seem just fine.
I'm okay with letting the current state of this PR make the choice.

—
Reply to this email directly or view it on GitHub
#61 (comment).

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

[SylvainCorlay]

@ellisonbg I updated the PR as per what we agreed on during the call.

[ellisonbg]

Just to update the community - on a phone call we reached consensus on using owner for this as this usage of more about a property pattern than a signaling one. If we ever have a signally abstraction we all agree that emitter would be good in that context.

[ellisonbg]

And great work!

[rmorshea]

It turns out that since this is being registered in the metaclass. The ObserveHandler is preventing the function from being registered as a method at runtime like it normally would. instance_init should instead read:

    def instance_init(self, inst):
        self.func = types.MethodType(self.func, inst)
        setattr(inst, self.name, self.func)
        for name in self.names:
            inst.observe(self.func, name= name)

[SylvainCorlay]

@rmorshea you are right. We should make it a bound method. Do you want to open a PR?

[sccolbert]

Yep, I didn't see that __call__ returns self.

No worries though, simply reimplement __get__ on the descriptor to return a new bound method when accessed:

def __get__(self, inst, cls):
    if inst is None:
        return self
    return MethodType(self.func, inst)

The way it currently sits, it allocates a new bound method for each function for each instance. It then also consumes more space by storing the method on the instance, plus circular ref, etc. That's pretty wasteful and can cause some undue pain when people start creating lots of objects.

[rmorshea]

I think that's a cleaner way to do it. What's the "circular ref" though, wouldn't the ObserveHander instance be garbage collected?

[sccolbert]

The circular ref would be caused by the bound method holding a reference to the instance, and instance holding a reference to the bound method. It's not the end of the world, and the garbage collector would probably get to it eventually (unless the instance has a __del__ method). That said, circular refs should be avoided when possible because they delay the cleanup of objects (forcing it upon the GC) and are generally a sign of a code-smell.

[rmorshea]

Gotcha, wasn't looking for it in the right place.