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

`post_execute` and `post_run_cell` hooks not always triggered #10774

Closed
fniephaus opened this Issue Sep 2, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@fniephaus
Contributor

fniephaus commented Sep 2, 2017

Due to various early returns in the code below, post_execute and post_run_cell hooks are not triggered in some cases. Since the pre hooks are always called, this is either a bug or the docs should mention that post hooks are not guaranteed to be triggered.

try:
code_ast = compiler.ast_parse(cell, filename=cell_name)
except self.custom_exceptions as e:
etype, value, tb = sys.exc_info()
self.CustomTB(etype, value, tb)
return error_before_exec(e)
except IndentationError as e:
self.showindentationerror()
if store_history:
self.execution_count += 1
return error_before_exec(e)
except (OverflowError, SyntaxError, ValueError, TypeError,
MemoryError) as e:
self.showsyntaxerror()
if store_history:
self.execution_count += 1
return error_before_exec(e)
# Apply AST transformations
try:
code_ast = self.transform_ast(code_ast)
except InputRejected as e:
self.showtraceback()
if store_history:
self.execution_count += 1
return error_before_exec(e)
# Give the displayhook a reference to our ExecutionResult so it
# can fill in the output value.
self.displayhook.exec_result = result
# Execute the user code
interactivity = "none" if silent else self.ast_node_interactivity
has_raised = self.run_ast_nodes(code_ast.body, cell_name,
interactivity=interactivity, compiler=compiler, result=result)
self.last_execution_succeeded = not has_raised
self.last_execution_result = result
# Reset this so later displayed values do not modify the
# ExecutionResult
self.displayhook.exec_result = None
self.events.trigger('post_execute')
if not silent:
self.events.trigger('post_run_cell')

/cc @craigcitro

@Carreau

This comment has been minimized.

Member

Carreau commented Sep 7, 2017

Thanks for the report, you are right that we should split the post_execution hooks into hooks that execute after every requests, or only after every valid Python request. I don't think we should fix the current ones, but provide variants that also trigger even when the pre-execute fails.

@Carreau Carreau added this to the 7.0 milestone Sep 7, 2017

@craigcitro

This comment has been minimized.

Contributor

craigcitro commented Sep 8, 2017

Hey @Carreau -- I'm curious about the details of what you're proposing.

  • Do you mean that you want to allow users to set separate hooks for the various ways a cell can fail, eg post_syntax_error_hook, post_preprocessing_error_hook, that sort of thing? I feel like that's a lot of complexity. For my money, I'd rather provide a way to customize the error_before_exec function that gets used.

  • Is the motivation for adding a new hook simply to avoid a subtle behavior change? As it is, I'm betting that most (if not all) users of the post_* hooks actually expect that they're always being executed, and would be surprised by the behavior @fniephaus ran into here. But of course I'm saying that based on 0 data. 😀 😀 😀 Do you know of a use case for the current behavior?

  • If we do go the route of new hooks, do you imagine having something like a post_execute_exception_hook, where we know exactly one of that and post_execute_hook will be triggered? Or something more like a finally_hook, where it's executed no matter what?

Thinking it through, I could see an argument that the right answer here is to say "we will be explicit about documenting and sticking to the current behavior, because it's a subtle behavior change. If you want something else, you should override run_cell yourself." I think that would work for our use case, but I bet we aren't the first and won't be the last to stumble on this, so we wanted to look at fixing it at the root first. 😉

@Carreau

This comment has been minimized.

Member

Carreau commented Sep 8, 2017

Do you mean that you want to allow users to set separate hooks for the various ways a cell can fail

Probably not that granular.

Is the motivation for adding a new hook simply to avoid a subtle behavior change?

Yes.

But of course I'm saying that based on 0 data

I see you are 0-indexed. And so we have at least 1 more data point: I would have expected these hooks to not trigger on failure :-)

If we do go the route of new hooks, do you imagine having something like a post_execute_exception_hook, where we know exactly one of that and post_execute_hook will be triggered? Or something more like a finally_hook, where it's executed no matter what?

Yes, probably a finally_hook, that get a parameter with whether things failed and how. Then it's explicit to user when it is called and give you all the flexibility you can get.

We'll better document anyway !

Thanks !

fniephaus added a commit to fniephaus/ipython that referenced this issue Sep 12, 2017

fniephaus added a commit to fniephaus/ipython that referenced this issue Sep 12, 2017

fniephaus added a commit to fniephaus/ipython that referenced this issue Sep 12, 2017

fniephaus added a commit to fniephaus/ipython that referenced this issue Sep 13, 2017

fniephaus added a commit to fniephaus/ipython that referenced this issue Sep 13, 2017

fniephaus added a commit to fniephaus/ipython that referenced this issue Sep 13, 2017

fniephaus added a commit to fniephaus/ipython that referenced this issue 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 ipython#10774.

@takluyver takluyver closed this in 1ce57fe Oct 18, 2017

@takluyver takluyver modified the milestones: 7.0, 6.3 Apr 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment