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

New events system #5188

Merged
merged 20 commits into from Mar 4, 2014
Merged

New events system #5188

merged 20 commits into from Mar 4, 2014

Conversation

takluyver
Copy link
Member

As discussed in #5122, there's a need for a richer set of callbacks. This is the initial architecture and a few callbacks.

  • ip.callbacks is an instance of CallbackManager (I know @ivanov will love this name)
  • There is a set of known events, each with a name, e.g. 'pre_execute_explicit', and a signature, defined by a prototype function in IPython.core.callbacks.
  • At present, there's no checking of signatures, but I've deliberately designed things so we could use something like backcall to verify them.
  • Callbacks are registered and unregistered by passing the event name and the callback function. There's no explicit provision for unregistering a callback without a reference to the function, but you can clear all callbacks for a given event.
  • Our code calls ip.callbacks.fire('event_name', *args, **kwargs). Again, there's not yet any checking that the args passed match the specified signature, but there easily could be in the future.
  • Exceptions raised by callbacks are displayed, but do not prevent other callbacks from executing.

I've deprecated a couple of hooks in favour of callbacks. Our hooks system is designed for end users to customise things that shouldn't be repeated if they succeed - e.g. 'load this in an editor' or 'get text from the clipboard'. In contrast, callbacks are for extension code listening for particular events, so it should be possible to register several callbacks for the same event without them interfering with one another.

@ellisonbg
Copy link
Member

Yeah! I haven't looked in detail, but I strongly think this is a great idea. Thanks so much for working on this. One comment about the naming of things. I agree that the callables being registered should be called "callbacks", but the actual abstraction is one of "events". This is almost the same exact design that we have/use in JavaScript and I think the naming of things should follow: events, trigger, on, rather than callbacks, fire and register. We also use the on style of things in Comms, Widgets, traitlets as well.

@jdfreder
Copy link
Member

something like backcall to verify them.

Shameless self advertising 😉

@takluyver
Copy link
Member Author

The best kind of self advertising ;-)

I'll look into giving things event-style names after lunch.

@takluyver
Copy link
Member Author

OK, it took a bit longer, but I've used 'events' and 'trigger' in the names.

I've left 'register' instead of 'on', which I don't like. 'on' seems to brief and generic to be useful e.g. when searching, and doesn't have a neat counterpart like 'unregister': jquery's use of 'off' is cute but silly.

@takluyver
Copy link
Member Author

As discussed in the dev meeting, I've added notes in the docs that this API is experimental for IPython 2.

@takluyver
Copy link
Member Author

From discussion with @minrk and @ivanov , the event names are now:

  • pre/post_execute fire for all code execution triggered by a frontend - execute requests and comm messages.
  • pre/post_run_cell fire when user code is run - execute requests with silent=False.

(In the plain terminal case, both also fire when running user entered code)

@ellisonbg
Copy link
Member

Needs rebase.

"""Manage a collection of events and a sequence of callbacks for each.

This is attached to :class:`~IPython.core.interactiveshell.InteractiveShell`
instances as a ``callbacks`` attribute.
Copy link
Member

Choose a reason for hiding this comment

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

as an events attribute

self.callbacks = {n:[] for n in available_events}

def register(self, event, function):
"""Register a new callback
Copy link
Member

Choose a reason for hiding this comment

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

a new event callback

# event_name -> prototype mapping
available_events = {}

def _collect(callback_proto):
Copy link
Member

Choose a reason for hiding this comment

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

I think the _collect name here is a bit vague. Maybe something like _define_event?

@takluyver
Copy link
Member Author

Rebased and changed those things.

# No-op functions which describe the names of available events and the
# signatures of callbacks for those events.
# ------------------------------------------------------------------------------

Copy link
Member

Choose a reason for hiding this comment

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

To the reader who is not very familiar with how the internals of IPython work, I think the names of these events are still very confusing (pre_execute, post_execute, pre_run_cell, post_run_cell) and the descriptions don't really help. It is also not clear the order in which these are triggered. Even if we don't rename them we should add better comments to clarify when they are triggered.

Copy link
Member

Choose a reason for hiding this comment

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

We should also clarify how silent affects these things.

Copy link
Member Author

Choose a reason for hiding this comment

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

We had a long discussion about this over lunch on Friday, and those were the clearest set of names we could come up with. Any alternative suggestions will be duly considered.

Copy link
Member

Choose a reason for hiding this comment

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

possible clarifications:

  • execute events fire before/after any code might be executed (interactive code, silent execution, widget interaction, comm messages).
  • run_cell events fire only before/after explicit "regular" execution, e.g. shift-enter or interactive typing at the terminal.

For those interested in the internal implementation, this means that silent=False execution is the only circumstance to trigger run_cell events. All potential execution triggers the execute events, so when run_cell fires, execute will also fire.

Copy link
Member Author

Choose a reason for hiding this comment

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

I find the wording a bit tricky - after all, the internal kernel code is running all the time, so it's not before/after 'any' code runs. I want to call it 'user defined' code, but the frontend can send code for execution without the user writing it, as is the case in most silent execution.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with the names, but let's add a comment about how silent affects which are called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a mention.

raise ValueError('argument %s must be callable' % func)
self._post_execute[func] = True
warn("ip.register_post_execute is deprecated, use "
"ip.callbacks.register('post_run_cell', func) instead.")
Copy link
Member

Choose a reason for hiding this comment

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

callbacks -> events like above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@ellisonbg
Copy link
Member

One non-inline question: do we need to update the pylab inline stuff to use this event system for rendering figures?

@ellisonbg
Copy link
Member

See IPython.core.pylabtools.configure_inline_support for things that need to be updated. A bit hackish right now with manual walking of the hook data stucture :(

@takluyver
Copy link
Member Author

Good point, that was actually what prompted these changes in the first place. I've switched it over.

@takluyver
Copy link
Member Author

Oops, missed a bit. Give me a moment to amend that commit. Github views don't auto update quickly enough.

@takluyver
Copy link
Member Author

OK, there we go. Less hackish now, although it's actually more lines of code in order to catch the exception. We should possibly consider having a pop-style API for unregister (i.e. ignore if not found). Or just wait for PEP 463 ;-)

try:
func(*args, **kwargs)
except Exception:
print("Error in callback {} (for {}):".format(func, event))
Copy link
Member

Choose a reason for hiding this comment

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

print to stderr, or no?

Copy link
Member Author

Choose a reason for hiding this comment

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

showtraceback() goes to stdout, looking at the implementation. Possibly it shouldn't, but this should go to the same stream, and changing showtraceback() is more invasive.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense.

@ellisonbg
Copy link
Member

I can confirm this fixes #5122 yeah!

@@ -827,12 +836,21 @@ def set_hook(self,name,hook, priority = 50, str_key = None, re_key = None):

setattr(self.hooks,name, dp)

#-------------------------------------------------------------------------
# Things related to callbacks
Copy link
Member

Choose a reason for hiding this comment

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

s/callbacks/events/

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

_remove_startswith(requires, pkg)
requires.append("gnureadline; sys.platform == 'darwin' and platform.python_implementation == 'CPython'")
requires.append("pyreadline (>=2.0); sys.platform == 'win32' and platform.python_implementation == 'CPython'")
requires.append("mock; extra == 'test'; python_version < '3.3'")
Copy link
Member

Choose a reason for hiding this comment

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

let's test whether ; or and should separate extra and platform conditions. Hopefully these are not both correct.

Copy link
Member

Choose a reason for hiding this comment

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

tested, and ; fails with SyntaxError. This should be and.

@minrk
Copy link
Member

minrk commented Mar 4, 2014

Excellent, merging.

minrk added a commit that referenced this pull request Mar 4, 2014
@minrk minrk merged commit 4c8ff1d into ipython:master Mar 4, 2014
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
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

5 participants