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

Ensure post event callbacks are always triggered. #10795

Merged
merged 6 commits into from Oct 18, 2017

Conversation

Projects
None yet
4 participants
@fniephaus
Copy link
Contributor

fniephaus commented Sep 13, 2017

This PR addresses #10774 by introducing two finally event callbacks: finally_execute and finally_run_cell. They work similar to the corresponding post callbacks, but are guaranteed to be triggered (even when, for example, a SyntaxError was raised). Also, the execution result is provided as an argument for further inspection.

The PR includes:

  • Code and tests
  • Docs (incl. example)
  • PR feature description

/cc @craigcitro

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Sep 13, 2017

API question for anyone interested: does it make more sense to have 'finally' callbacks that fire regardless of whether code is executed, or 'error' callbacks that only fire when the 'post' ones wouldn't? It should be possible to do the same things with either API, but I wonder if there's any design reason to prefer one over the other.

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

fniephaus commented Sep 13, 2017

First, 'post' callbacks don't get the result as an argument, 'error' callbacks would probably just expect the error as an argument. Therefore, the current version of 'finally' callbacks is more powerful as it allows to inspect the execution result.
Also, the docs on 'post' callbacks states that they can be used for, for example, cleanups etc. Let's just say a 'pre' callback creates a file. If you want that to be removed, you'd need to add the logic to both, 'post' and 'error' callbacks. 'Finally' callbacks seem to be an easy way to cover all use cases, as it's simple to check whether an error was thrown or not (see example).

@takluyver
Copy link
Member

takluyver left a comment

Thanks, I'm convinced about doing finally_ events rather than error_ events. A few other things I'd like to consider:

  1. Maybe it's actually fine to make the post_ events fire in all the cases where the new finally_ events do. I don't know if there are any real use cases that would be broken by firing them after a syntax error.
  2. We could actually add parameters to existing events... I wrote a little package a few years back to deal with precisely that: backcall. I don't think it's ever been seriously used, so it might be rough, but I was thinking of IPython's events API when I wrote it.
  3. Naming: finally_execute will fire in cases where code has not been executed. Maybe it should just be finally_run_cell and take silent as another parameter.
For instance, the inline matplotlib backend uses this event to display any figures created but not explicitly displayed during the course of the cell.

``post_run_cell`` runs after successful interactive execution (e.g. a cell in a
notebook, but, for example, not when a ``SyntaxError`` was raised).

This comment has been minimized.

@takluyver

takluyver Sep 15, 2017

Member

If we leave the post_ events as they are (I think we should consider changing them), this documentation needs to be a bit more precise. They still fire after a normal error (e.g. from 1/0), it's only if preprocessing/parsing/compiling the code fails, so we don't actually execute it, that they don't fire.

This comment has been minimized.

@fniephaus

fniephaus Sep 18, 2017

Author Contributor

Sure, will update this in the next commit...

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

fniephaus commented Sep 18, 2017

  1. Maybe it's actually fine to make the post_ events fire in all the cases where the new finally_ events do. I don't know if there are any real use cases that would be broken by firing them after a syntax error.
  2. We could actually add parameters to existing events... I wrote a little package a few years back to deal with precisely that: backcall. I don't think it's ever been seriously used, so it might be rough, but I was thinking of IPython's events API when I wrote it.

I'm not familiar with IPython's backwards compatibility policy, but introducing finally_ events appears to be safer than changing the semantics of existing events. Also, adding parameters to existing events would cause method signature mismatches, which might also not be great in terms of compatibility. What other parameters were you thinking about?

  1. Naming: finally_execute will fire in cases where code has not been executed. Maybe it should just be finally_run_cell and take silent as another parameter.

Purely for consistency reasons, I'm still in favor of adding both, finally_execute and finally_run_cell.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Sep 18, 2017

adding parameters to existing events would cause method signature mismatches

That's precisely what backcall is designed to handle - you can add parameters and callbacks that don't accept them will still work. I agree that it's reasonable to just add new events, though.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Sep 18, 2017

I'd be happy if this were the behavior of the existing events and backcall were used for backward-compatibility of the signature. I imagine that most people writing current event handlers would expect this behavior rather than the current. I know that most times that I've written execute hooks I wished that the cell text were part of the callback signature.

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

fniephaus commented Sep 20, 2017

Okay, so what do we want to do? Modify and extend existing callbacks or introducing new ones?
I'm personally not a big fan of adding more complexity in cases like this, so I'd still be in favor of adding new callbacks. Otherwise, I'm happy to incorporate backcall's approach to provide backwards compatibility.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Sep 27, 2017

I'd be in favour of trying out backcall to extend existing callbacks, but it's my weird side project, so I shouldn't be the one making that call. @minrk? @Carreau ?

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Sep 28, 2017

👍 to giving backcall a go and applying this to the existing events.

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

fniephaus commented Oct 1, 2017

Ok, cool. Will update this PR accordingly later this week...

fniephaus added some commits Oct 5, 2017

Ensure post event callbacks are always called.
Also, provide the execution result object as an argument for both *pre* and *post* event callbacks in a backward compatible manner.

Closes #10774.

@fniephaus fniephaus changed the title Add support for "finally" event callbacks. Ensure post event callbacks are always triggered. Oct 5, 2017

@fniephaus fniephaus force-pushed the fniephaus:finally-hooks branch 2 times, most recently from 9afd02c to 3650024 Oct 5, 2017

@fniephaus fniephaus force-pushed the fniephaus:finally-hooks branch from 3650024 to 8ed324e Oct 5, 2017

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

fniephaus commented Oct 5, 2017

PTAL, I have:

  • Adjusted the behavior of the post callbacks as discussed;
  • Removed the finally callbacks again;
  • Added the result object as an argument to both pre and post callbacks. Because we would need to use a backport of inspect.signature for Python2, I've decided to use a much simpler approach instead: the previous versions of all pre and post callbacks had no arguments, so compatibility wrappers are only used for callback functions without an argument.
@Carreau

This comment has been minimized.

Copy link
Member

Carreau commented Oct 5, 2017

Thanks all for restarting that and following up, I'm slowly getting back to review PR and push IPython forward, apologies for the delay. I'll try to catch up with the discussion.

Because we would need to use a backport of inspect.signature for Python2,

Unless you want this PR back ported to the 5.x branch that will be unnecessary as IPython is already 3.3+ already. I do understand though that backporting could help to make external code easier to maintain on two version of IPython.

@Carreau

Carreau approved these changes Oct 5, 2017

Copy link
Member

Carreau left a comment

+1, and I believe that should be relatively easy to backport.

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

fniephaus commented Oct 5, 2017

Unless you want this PR back ported to the 5.x branch that will be unnecessary as IPython is already 3.3+ already. I do understand though that backporting could help to make external code easier to maintain on two version of IPython.

I don't think we need this code to be back ported to the 5.x branch. But since the current version uses a simple import fallback for Python2 compatibility and does not depend on inspect.signature, it should be easy to do as you already mentioned.

@@ -90,29 +125,53 @@ def _define_event(callback_proto):
# ------------------------------------------------------------------------------

@_define_event
def pre_execute():
def pre_execute(result):

This comment has been minimized.

@takluyver

takluyver Oct 11, 2017

Member

It seems strange for the pre events to carry a result object. Also, the code in interactiveshell that triggers these events does not give them the result.

This comment has been minimized.

@fniephaus

fniephaus Oct 11, 2017

Author Contributor

Opps, looks like I forgot to provide the result as an argument. Anyway, if you don't want the pre events that carry as result object, I can clean that up again. I just thought it would be useful, so user for example can "memorize" a result in a pre event and then match them in the corresponding post event (stupid example: pre event allocates memory for a computation, post event frees that corresponding memory allocation again if that makes sense).
Remove or keep the result argument?

This comment has been minimized.

@takluyver

takluyver Oct 11, 2017

Member

Oh, just using it as a handle to match up events? I guess so, but I find it hard to think of a real case where you'd want that. @minrk?

This comment has been minimized.

@minrk

minrk Oct 11, 2017

Member

I would expect the pre events would take the requested code to be executed (basically the ExecuteRequest from the message perspective), i.e. a dict of run_cell's kwargs. I've encountered several places where people want pre-execute notifiers of executions about to start.

For matching things up, since execution is ordered, there should always be one post_execute after each pre_execute, never two pre_executes in a row, so I'm not sure there is a use case for the pre_execute getting the incomplete result object. I, too, found it a little surprising to have a result object prior to the execution occurring.

This comment has been minimized.

@fniephaus

fniephaus Oct 12, 2017

Author Contributor

Thanks for the comments. I've introduced ExecuteRequest which does what @minrk suggested. Additionally, I've added a reference to the corresponding ExecuteRequest to ExecuteResults which allows to retrieve for example the cell code in post events as well (and also to match up events if this is actually a use case).

len(getfullargspec(function).args) == 0):
# `callback_proto` has args but `function` does not, so a
# compatibility wrapper is needed.
self.callbacks[event].append(_compatibility_wrapper_for(function))

This comment has been minimized.

@takluyver

takluyver Oct 11, 2017

Member

Is there a good reason to implement this functionality within IPython rather than adding a dependency on backcall and using that? We can improve backcall if needed, but it's more flexible than this implementation - e.g. you can have a callback that gets only some of the arguments available.

This comment has been minimized.

@fniephaus

fniephaus Oct 11, 2017

Author Contributor

I didn't know that adding backcall as a dependency is an option and that you'd prefer that. I just wanted to replicate the behavior we needed for this feature here without adding to much overhead. Will update the PR again when I have some time...

@fniephaus fniephaus force-pushed the fniephaus:finally-hooks branch 7 times, most recently from 59b44a3 to b9fff24 Oct 12, 2017

@fniephaus fniephaus force-pushed the fniephaus:finally-hooks branch from b9fff24 to de7565e Oct 12, 2017

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

fniephaus commented Oct 12, 2017

backcall is now used as a dependency, so PTAL :)

@fniephaus fniephaus force-pushed the fniephaus:finally-hooks branch from de7565e to 2c81acc Oct 12, 2017

@minrk

minrk approved these changes Oct 13, 2017

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Oct 13, 2017

I'll give @takluyver the last word, but looks AOK to me. Thanks!

@takluyver takluyver added this to the 6.3 milestone Oct 13, 2017

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 13, 2017

The API looks good, but I think the implementation could be simplified a little bit.

This maintains a global mapping of (event, function) pairs to the wrapped functions. Am I right in thinking that the only need for this is to allow unregistering functions again, by looking up the wrapped function to remove?

If so, I think we can do it without keeping a mapping by using the .__wrapped__ attribute of the wrapper function, something like this:

def unregister(self, event, function):
    if function in self.callbacks[event]:
        return self.callbacks[event](remove)

    for callback in self.callbacks[event]:
        if callback.__wrapped__ is function:
            return self.callbacks[event].remove(callback)

    raise ValueError("Function {!r} is not registered as a {} callback".format(function, event)
@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 13, 2017

Also, a naming question. I think ExecutionRequest could cause confusion with an execute_request message - it might have come from such a message, but it doesn't directly represent one.

I'm not sure what a better name is, though. ExecutionParams?

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 13, 2017

Ohhh, also, I've just remembered something. We can add these parameters to pre_run_cell and post_run_cell, but we can't add them to pre_execute and post_execute; the latter events are also fired around the callbacks for comm messages, which happen without all the context of run_cell.

https://github.com/ipython/ipykernel/blob/71c44fe80e79126de4f27e6713fda1d274132d77/ipykernel/comm/comm.py#L152-L161

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

fniephaus commented Oct 13, 2017

re #10795 (comment): yes, you are right @takluyver. Will make that better...

re #10795 (comment): Oh right, I forgot about execute_request. ExecutionParams might be a little weird, because it would read pre_run_cell(params), I would expect pre_run_cell(raw_cell, silent, ...). On the other hand, the latter would be inconsistent with regard to the post event. What do you think about something more generic like ExecutionInfo or ExecutionData?

re #10795 (comment): I didn't know about handle_msg. Shouldn't there be pre_handle_msg and post_handle_msg which take the msg as an argument? Or shall we provide an ExecutionRequest with a reference to msg instead?

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 13, 2017

I'd be OK with ExecutionInfo, or maybe ExecutionInput. @minrk @Carreau - any brainwaves for the name of the type to store data about a call to run_cell(), for event handlers?

I think we'll just have to only add the data to pre_run_cell and post_run_cell. Changing the data for the *_execute events is going to be too messy now that they're called from two different projects with separate release schedules, and there's no good analogue of the ExecutionResult or ExecutionRequest (whatever that gets called) when we're handling a comm message.

@fniephaus fniephaus force-pushed the fniephaus:finally-hooks branch 2 times, most recently from f2b38b6 to 7d99285 Oct 18, 2017

@fniephaus fniephaus force-pushed the fniephaus:finally-hooks branch from 7d99285 to 7de483b Oct 18, 2017

@fniephaus

This comment has been minimized.

Copy link
Contributor Author

fniephaus commented Oct 18, 2017

@takluyver great, so I've renamed the class to ExecutionInfo, removed it as an argument from the *execute events and refactored the code as you suggested. Is there anything else you want me to change?

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Oct 18, 2017

Yes, I'm happy with this. Thanks for your patience with all the changes I asked for!

@takluyver takluyver merged commit 0dbfdb9 into ipython:master Oct 18, 2017

4 checks passed

codecov/patch 91.3% of diff hit (target 0%)
Details
codecov/project 67.07% (+0.04%) compared to d29752e
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fniephaus

This comment has been minimized.

Copy link
Contributor Author

fniephaus commented Oct 18, 2017

Thanks for the comments!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.