Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make ip.reset() work the same in interactive or non-interactive code. #347

Closed
wants to merge 1 commit into from

3 participants

@takluyver
Owner

At present, ip.reset() sets execution_count to 0. If that's done interactively, it gets bumped up by 1 as it leaves run_cell, so the next prompt is 1. But in test code, we call ip.reset() outside of run_cell, so the next run_cell command executes as cell 0, leading to unexpected problems.

This makes ip.reset() set execution_count to 1, and set a boolean flag so that run_cell doesn't increment it immediately afterwards.

@minrk
Owner

Thanks, I don't see any problem with this.

@fperez
Owner

I'm not crazy about the solution, though I can't think of a cleaner one right now. What I don't like is that we're solving the problem by introducing a flag to sync various functions, and we now have to track that flag around. That means more special-case code and coupling between different parts of the code.

Ideally, we should think through the execution count logic so that these scenarios 'just work', rather than adjusting counters up and down based on flags...

In fact, I'm not even sure that reset should set the counter back to 0 (or 1): it gives a false sense of security because the kernel hasn't really been fully reset (extension code is still there, you may have trapped references in tracebacks, etc). So I'm actually inclined to flush all variables, but leave the counter where it was, as a sign to the user that they may have a mostly cleaned up namespace but they're still sitting on an old kernel.

The qt console can do a real kernel restart, and when we have a 2-process terminal one we'll be able to do the same, but for now I'm inclined to say that %reset should just leave that counter alone... Thoughts?

@minrk
Owner

Since reset doesn't really reset, maybe it just shouldn't touch the execution counter.

@takluyver
Owner

This is the code that gets called for a hard reset, i.e. we're aiming to completely nuke all references, and return the interpreter to a 'just started' state. If there's anything we're not clearing up in this, I consider it a bug. We also get a new history session, so it makes sense to me that the line numbers should go from 1 again. For a soft reset (%reset -s), we leave the execution_count untouched.

I know this isn't perfect, but it was the cleanest solution I could think of. The alternative would be to increment the execution_count before running the user code, and then have displayhook use execution_count-1, but that just feels untidy. And when I looked at the logic to produce the numbers in the prompts, I got lost in layers of unfamiliar code.

In the future, there may also be other functions we can call interactively which change the prompt number (I'm thinking if you want to do things between sessions), in which case the same problem will come up.

@minrk
Owner

This actually points to a bigger problem:

In [1]: get_ipython().shell.run_cell('a=5')
ERROR! Session/line number was not unique in database. History logging moved to new session 648
Out[2]: False

In [3]: _1
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
/Users/minrk/ in ()
----> 1 _1

NameError: name '_1' is not defined

In [4]: _2
Out[4]: False

The execution count should probably be incremented at the top of run_cell, which should prevent this sort of thing.

@fperez
Owner
@takluyver
Owner

Min: any code that can call back into run_cell from interactive user code should use store_history=False for precisely that reason. I think I've tidied up all the obvious cases.

Fernando: OK, fair enough. I'll change %reset so it doesn't reset the execution count.

@takluyver takluyver closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 10 additions and 3 deletions.
  1. +10 −3 IPython/core/interactiveshell.py
View
13 IPython/core/interactiveshell.py
@@ -1050,7 +1050,7 @@ def reset(self, new_session=True):
self.displayhook.flush()
# Reset counter used to index all histories
- self.execution_count = 0
+ self.execution_count = 1
# Restore the user namespaces to minimal usability
for ns in self.ns_refs_table:
@@ -1076,6 +1076,10 @@ def reset(self, new_session=True):
# Flush the private list of module references kept for script
# execution protection
self.clear_main_mod_cache()
+
+ # If this is run from interactive code, we don't want run_cell to bump
+ # up the execution_count
+ self.increment_exec_count = False
def reset_selective(self, regex=None):
"""Clear selective variables from internal namespaces based on a
@@ -2121,6 +2125,7 @@ def run_cell(self, cell, store_history=True):
should be set to False.
"""
raw_cell = cell
+ self.increment_exec_count = True
with self.builtin_trap:
cell = self.prefilter_manager.prefilter_lines(cell)
@@ -2139,7 +2144,8 @@ def run_cell(self, cell, store_history=True):
except (OverflowError, SyntaxError, ValueError, TypeError, MemoryError):
# Case 1
self.showsyntaxerror()
- self.execution_count += 1
+ if store_history and self.increment_exec_count:
+ self.execution_count += 1
return None
interactivity = 'last' # Last node to be run interactive
@@ -2153,7 +2159,8 @@ def run_cell(self, cell, store_history=True):
# history output logging is enabled.
self.history_manager.store_output(self.execution_count)
# Each cell is a *single* input, regardless of how many lines it has
- self.execution_count += 1
+ if self.increment_exec_count:
+ self.execution_count += 1
def run_ast_nodes(self, nodelist, cell_name, interactivity='last'):
"""Run a sequence of AST nodes. The execution mode depends on the
Something went wrong with that request. Please try again.