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

Deprecate on_trait_change and magic change handlers #61

Merged
merged 8 commits into from
Aug 27, 2015

Conversation

SylvainCorlay
Copy link
Member

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

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

- object : the HasTraits instance
- old : the old value of the modified trait attribute
- new : the new value of the modified trait attribute
- name : the name ofthe modified trait attribute.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space

@SylvainCorlay
Copy link
Member Author

@ellisonbg @sccolbert as per earlier discussion.

@blink1073
Copy link
Member

@SylvainCorlay, tested both constructs, 👍

@blink1073
Copy link
Member

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

@SylvainCorlay
Copy link
Member Author

Hum, I am seeing the warnings in the notebook.

@blink1073
Copy link
Member

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

@jasongrout
Copy link
Member

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.

@@ -547,6 +547,51 @@ def tag(self, **metadata):
# The HasTraits implementation
#-----------------------------------------------------------------------------

class _CbWrapper(object):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

handler : callable
A callable that is called when a trait changes. Its
signature can be handler() or handler(change), where change is a
dictionary with the following optional keys:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the keys really optional?

@jasongrout
Copy link
Member

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

@SylvainCorlay SylvainCorlay changed the title [WIP] Deprecate on_trait_change and magic change handlers Deprecate on_trait_change and magic change handlers Aug 7, 2015
@SylvainCorlay
Copy link
Member Author

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

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

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

@ellisonbg
Copy link
Member

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

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

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

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

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

@SylvainCorlay
Copy link
Member Author

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

@ellisonbg
Copy link
Member

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

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

@minrk
Copy link
Member

minrk commented Aug 27, 2015 via email

@ellisonbg
Copy link
Member

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

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

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

@ellisonbg
Copy link
Member

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 added a commit that referenced this pull request Aug 27, 2015
Deprecate on_trait_change and magic change handlers
@ellisonbg ellisonbg merged commit 9739285 into ipython:master Aug 27, 2015
@ellisonbg
Copy link
Member

And great work!

@minrk minrk added this to the 4.1 milestone Aug 27, 2015
self.func = func
return self

def instance_init(self, inst):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 SylvainCorlay deleted the observe branch August 27, 2015 22:07
@SylvainCorlay
Copy link
Member Author

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

@sccolbert
Copy link

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

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

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

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

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

Successfully merging this pull request may close these issues.

None yet

9 participants