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

Allow starting TerminalInteractiveShell more than once #9843

Merged
merged 5 commits into from Aug 10, 2016

Conversation

Projects
None yet
3 participants
@minrk
Member

minrk commented Aug 5, 2016

  1. always start with keep_running=True in interact(), which avoids interact() being a no-op on all runs after the first.
  2. don't close eventloop at end of mainloop (not sure this is the right thing to do)
  3. pass self, not self.Completer to PTCompleter, to ensure that it's always hooked up to shell.Completer, which can be replaced over time (pudb).

cc @Carreau, @takluyver for the right thing to do about _eventloop.close(), since I doubt this is the right thing. Maybe register close with atexit.register?

cc @inducer this fixes inducer/pudb#193

Allow starting TerminalInteractiveShell more than once
1. always start with keep_running=True in `interact()`
2. don't close eventloop at end of mainloop (not sure this is the right thing to do)

@minrk minrk referenced this pull request Aug 5, 2016

Closed

Problems with IPython 5 #193

give PTCompleter InteractiveShell, not Completer
it is possible for the completer to get reloaded/replaced,
at which point the prompt-toolkit completions will not be those of ip.Completer,
but whatever ip.Completer was originally.
Show outdated Hide outdated IPython/terminal/ptutils.py
@@ -12,8 +12,12 @@
class IPythonPTCompleter(Completer):
"""Adaptor to provide IPython completions to prompt_toolkit"""
def __init__(self, ipy_completer):
self.ipy_completer = ipy_completer
def __init__(self, shell):

This comment has been minimized.

@Carreau

Carreau Aug 5, 2016

Member

gruml gluml change of API.. gruml, gruml but I see the reason.
Can we check the type of shell and print a deprecation warning if it's a Completer ?

@Carreau

Carreau Aug 5, 2016

Member

gruml gluml change of API.. gruml, gruml but I see the reason.
Can we check the type of shell and print a deprecation warning if it's a Completer ?

This comment has been minimized.

@minrk

minrk Aug 8, 2016

Member

Not every internal utility class is a public API, so I have no qualms changing this one. I can make it backward-compatible, though.

@minrk

minrk Aug 8, 2016

Member

Not every internal utility class is a public API, so I have no qualms changing this one. I can make it backward-compatible, though.

This comment has been minimized.

@minrk

minrk Aug 8, 2016

Member

Added now in a fully backward-compatible way, and explicitly declared the module as private APIs.

@minrk

minrk Aug 8, 2016

Member

Added now in a fully backward-compatible way, and explicitly declared the module as private APIs.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Aug 5, 2016

Member

cc @Carreau, @takluyver for the right thing to do about _eventloop.close(), since I doubt this is the right thing. Maybe register close with atexit.register?

Probably, or should we have an event loop "reference counting" and a with eventloop context manager that hold the eventloop ?

Member

Carreau commented Aug 5, 2016

cc @Carreau, @takluyver for the right thing to do about _eventloop.close(), since I doubt this is the right thing. Maybe register close with atexit.register?

Probably, or should we have an event loop "reference counting" and a with eventloop context manager that hold the eventloop ?

minrk added some commits Aug 8, 2016

Make IPythonPTCompleter(shell) backward-compatible
and declare that ptutils only contains private APIs
"""prompt-toolkit utilities
Everything in this module is a private API,
not to be used outside IPython.

This comment has been minimized.

@takluyver

takluyver Aug 8, 2016

Member

Let's add it to the packages skipped for the API docs, in the list here.

@takluyver

takluyver Aug 8, 2016

Member

Let's add it to the packages skipped for the API docs, in the list here.

This comment has been minimized.

@takluyver

takluyver Aug 8, 2016

Member

In fact, here.

@takluyver

takluyver Aug 8, 2016

Member

In fact, here.

This comment has been minimized.

@minrk

minrk Aug 8, 2016

Member

Done.

@minrk

minrk Aug 8, 2016

Member

Done.

@minrk minrk added this to the 5.1 milestone Aug 8, 2016

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 8, 2016

Member

should we have an event loop "reference counting" and a with eventloop context manager that hold the eventloop ?

I don't think that quite works because it seems like the refcount would never be anything other than one after the class is instantiated, since the loop is setup in __init__, and shouldn't be closed until the object is garbage collected (atexit or __del__). It's not recursive, so the count doesn't go up, it's starting and stopping the same IPython instance once in a while.

Member

minrk commented Aug 8, 2016

should we have an event loop "reference counting" and a with eventloop context manager that hold the eventloop ?

I don't think that quite works because it seems like the refcount would never be anything other than one after the class is instantiated, since the loop is setup in __init__, and shouldn't be closed until the object is garbage collected (atexit or __del__). It's not recursive, so the count doesn't go up, it's starting and stopping the same IPython instance once in a while.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 8, 2016

Member

I've just had a look at the prompt_toolkit code, and it doesn't look like there should be any ill effects of not calling eventloop.close(). Some fds will remain open, but you probably only have one InteractiveShell object, so a few fds lingering shouldn't be a problem.

If we do call it anywhere, I think __del__ would be the best place. That assumes that no-one is taking a reference to the event loop and using it outside of the shell object, but I think that's probably safe. Or maybe prompt_toolkit should have a __del__ method on the event loop classes to clear up their fds. (@jonathanslenders)

Member

takluyver commented Aug 8, 2016

I've just had a look at the prompt_toolkit code, and it doesn't look like there should be any ill effects of not calling eventloop.close(). Some fds will remain open, but you probably only have one InteractiveShell object, so a few fds lingering shouldn't be a problem.

If we do call it anywhere, I think __del__ would be the best place. That assumes that no-one is taking a reference to the event loop and using it outside of the shell object, but I think that's probably safe. Or maybe prompt_toolkit should have a __del__ method on the event loop classes to clear up their fds. (@jonathanslenders)

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 9, 2016

Member

I tried putting it in __del__, but it doesn't seem to ever be called, perhaps due to circular references/global links created between pt and/or IPython.

Member

minrk commented Aug 9, 2016

I tried putting it in __del__, but it doesn't seem to ever be called, perhaps due to circular references/global links created between pt and/or IPython.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 9, 2016

Member

There certainly are going to be circular references through IPythonPTCompleter and TerminalInteractiveShell.inputhook. But recent versions of Python should be able to break the cycle and call __del__.

Member

takluyver commented Aug 9, 2016

There certainly are going to be circular references through IPythonPTCompleter and TerminalInteractiveShell.inputhook. But recent versions of Python should be able to break the cycle and call __del__.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 9, 2016

Member

Python 3.5 isn't recent enough, at least.

Member

minrk commented Aug 9, 2016

Python 3.5 isn't recent enough, at least.

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Aug 10, 2016

Member

I've just tested with a __del__ method on TerminalInteractiveShell, and it does get called when the interpreter (3.5) exits. I was doing this to check that I hadn't misunderstood something. I don't think it's terribly important to do cleanup in __del__, and it would be better done in Eventloop.__del__ anyway. So I'm merging this as is.

Member

takluyver commented Aug 10, 2016

I've just tested with a __del__ method on TerminalInteractiveShell, and it does get called when the interpreter (3.5) exits. I was doing this to check that I hadn't misunderstood something. I don't think it's terribly important to do cleanup in __del__, and it would be better done in Eventloop.__del__ anyway. So I'm merging this as is.

@takluyver takluyver merged commit f03536e into ipython:master Aug 10, 2016

3 checks passed

codecov/patch 36.36% of diff hit (target 0.00%)
Details
codecov/project 73.55% (-0.02%) compared to 345bf7d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@minrk minrk deleted the minrk:reentrant-pt branch Aug 10, 2016

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Aug 10, 2016

Member

've just tested with a del method on TerminalInteractiveShell, and it does get called when the interpreter (3.5) exits

I did the same and it didn't (and again just now, with conda CPython 3.5.1 on OS X), so it's at least not a trustworthy event.

Agreed on the other points, though. Thanks!

Member

minrk commented Aug 10, 2016

've just tested with a del method on TerminalInteractiveShell, and it does get called when the interpreter (3.5) exits

I did the same and it didn't (and again just now, with conda CPython 3.5.1 on OS X), so it's at least not a trustworthy event.

Agreed on the other points, though. Thanks!

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