Fix IPython kernel-related cleanup #188

mdickinson · 2019-09-10T06:41:39Z

This PR shows a proof of concept for fixing cleanup issues in the InternalIPKernel and associated plugin. Should eventually address issues #94, #185, #186 and #187.

~~Currently WIP, so making this a draft PR. Opening a PR now (a) to get CI feedback and (b) to have a place outside the code to record things that are still to do.~~

Ready for review.

To do:

Lots of cleanup and removal of duplicated code.
Tests.
- Test that sys.std*, sys.excepthook and sys.displayhook are all restored as expected
- Test that IPython.utils.io globals unaltered
- Test that no ZMQInteractiveShell or IPythonKernel objects remain after cleanup
Absorb changes from Redirect IPython's raw_print and raw_print_err to logging. #183 (they need extending to cover rprint and rprinte, which are still used in some versions of the code).
zmq Sockets are only being cleaned up through cyclic gc. Can we clean them up better?
investigate and fix the ResourceWarning from test_shutdown_closes_console_pipes

Note: commit history is very messy. Please be sure to squash and merge.

… code).

… the trait.

codecov-io · 2019-09-10T06:51:53Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4b8431c). Click here to learn what that means.
The diff coverage is 92.15%.

@@            Coverage Diff            @@
##             master     #188   +/-   ##
=========================================
  Coverage          ?   45.73%           
=========================================
  Files             ?      147           
  Lines             ?     4476           
  Branches          ?      506           
=========================================
  Hits              ?     2047           
  Misses            ?     2384           
  Partials          ?       45

Impacted Files	Coverage Δ
...visage/plugins/ipython_kernel/internal_ipkernel.py	`93.18% <100%> (ø)`
envisage/plugins/ipython_kernel/heartbeat.py	`81.81% <81.81%> (ø)`
envisage/plugins/ipython_kernel/kernelapp.py	`92.18% <92.18%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b8431c...ab5b258. Read the comment docs.

…nel to kernelapp

envisage/plugins/ipython_kernel/internal_ipkernel.py

…ove some unnecessary cleanup

…n Python 2.

…tdout caching to init_kernel.

mdickinson · 2019-09-10T13:04:50Z

I'm getting a ResourceWarning from one of the tests:

test_shutdown_closes_console_pipes (envisage.plugins.ipython_kernel.tests.test_internal_ipkernel.TestInternalIPKernel) ... 
To connect another client to this kernel, use:
    --existing kernel-95701.json
/Users/mdickinson/.edm/envs/envisage-test-3.6-null/lib/python3.6/subprocess.py:766: ResourceWarning: subprocess 95708 is still running
  ResourceWarning, source=self)
ok

…on test failure.

mdickinson · 2019-09-10T13:54:01Z

The ResourceWarning is a result of a semi-bug: when we clean up the consoles, we do a console.term() for each console, but don't then do a console.wait(); I've added that missing wait call, which sets the return code on the Popen object, which in turn allows the subprocess module to recognise that the child process has finished, and prevents the ResourceWarning from being issued.

envisage/plugins/ipython_kernel/internal_ipkernel.py

rahulporuri · 2019-09-11T08:38:17Z

I have no additional comments on this PR and any questions I had have been addressed.

envisage/plugins/ipython_kernel/heartbeat.py

envisage/plugins/ipython_kernel/internal_ipkernel.py

corranwebster · 2019-09-11T08:59:07Z

envisage/plugins/ipython_kernel/tests/test_internal_ipkernel.py

+# the existence and content of those messages, and for hiding the
+# logger output during the test run).
+
+class ListHandler(logging.Handler):


This feels generally useful, and may belong in apptools.logging.

Code is now gone; I'll try to remember to open an apptools issue for this and for the output stream capturing before I close this PR.

Opened enthought/apptools#96

corranwebster

Generally looks good, with the caveat that I haven't done the digging into IPython to ensure that clean up is doing all the right things, but it looks reasonable and the tests seem comprehensive enough.

Furthermore, I think that architecturally it is the right sort of thing.

…low this.

…ctrl_c message.

mdickinson · 2019-09-12T10:18:32Z

There's something a bit fragile with the current tests and latest ipykernel and friends from PyPI. I have an intermittent exception that looks like this:

test_no_threads_leaked (envisage.plugins.ipython_kernel.tests.test_internal_ipkernel.TestInternalIPKernel) ... Exception in thread Thread-14:
Traceback (most recent call last):
  File "/Users/mdickinson/.edm/envs/envisage/lib/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/Users/mdickinson/Enthought/ETS/envisage/envisage/plugins/ipython_kernel/heartbeat.py", line 37, in run
    super(Heartbeat, self).run()
  File "/Users/mdickinson/.edm/envs/envisage/lib/python3.6/site-packages/ipykernel/heartbeat.py", line 91, in run
    self._bind_socket()
  File "/Users/mdickinson/.edm/envs/envisage/lib/python3.6/site-packages/ipykernel/heartbeat.py", line 74, in _bind_socket
    self._try_bind_socket()
  File "/Users/mdickinson/.edm/envs/envisage/lib/python3.6/site-packages/ipykernel/heartbeat.py", line 61, in _try_bind_socket
    return self.socket.bind('%s://%s' % (self.transport, self.ip) + c + str(self.port))
  File "zmq/backend/cython/socket.pyx", line 550, in zmq.backend.cython.socket.Socket.bind
  File "zmq/backend/cython/checkrc.pxd", line 25, in zmq.backend.cython.checkrc._check_rc
zmq.error.ZMQError: Address already in use

ok

… and ctrl_c_message.

mdickinson · 2019-09-12T10:53:18Z

Variant on the intermittent failure:

test_io_pub_thread_stopped (envisage.plugins.ipython_kernel.tests.test_internal_ipkernel.TestInternalIPKernel) ... Unhandled exception in thread started by <bound method Thread._bootstrap of <Heartbeat(Thread-4, started daemon 123145440456704)>>
Traceback (most recent call last):
  File "/Users/mdickinson/.edm/envs/envisage/lib/python3.6/threading.py", line 916, in _bootstrap_inner
    self.run()
  File "/Users/mdickinson/Enthought/ETS/envisage/envisage/plugins/ipython_kernel/heartbeat.py", line 30, in run
    super(Heartbeat, self).run()
  File "/Users/mdickinson/.edm/envs/envisage/lib/python3.6/site-packages/ipykernel/heartbeat.py", line 91, in run
    self._bind_socket()
  File "/Users/mdickinson/.edm/envs/envisage/lib/python3.6/site-packages/ipykernel/heartbeat.py", line 74, in _bind_socket
    self._try_bind_socket()
  File "/Users/mdickinson/.edm/envs/envisage/lib/python3.6/site-packages/ipykernel/heartbeat.py", line 61, in _try_bind_socket
    return self.socket.bind('%s://%s' % (self.transport, self.ip) + c + str(self.port))
  File "zmq/backend/cython/socket.pyx", line 550, in zmq.backend.cython.socket.Socket.bind
  File "zmq/backend/cython/checkrc.pxd", line 25, in zmq.backend.cython.checkrc._check_rc
zmq.error.ZMQError: Address already in use

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/mdickinson/.edm/envs/envisage/lib/python3.6/threading.py", line 884, in _bootstrap
    self._bootstrap_inner()
  File "/Users/mdickinson/.edm/envs/envisage/lib/python3.6/threading.py", line 926, in _bootstrap_inner
    (self.name, _format_exc()), file=_sys.stderr)
  File "/Users/mdickinson/.edm/envs/envisage/lib/python3.6/site-packages/ipykernel/iostream.py", line 394, in write
    raise ValueError('I/O operation on closed file')
ValueError: I/O operation on closed file
ok

mdickinson · 2019-09-12T12:45:42Z

@rahulporuri @corranwebster I think this is ready. I'm still investigating the intermittent failure, but it may make most sense for the fix for that to go into a separate PR.

mdickinson · 2019-09-12T14:00:39Z

I'm still investigating the intermittent failure

This turned out to be an ipykernel bug; see ipython/ipykernel#430. We'll need to figure out how to work around it here, but that can happen in another PR.

mdickinson · 2019-09-13T07:20:53Z

We'll need to figure out how to work around it here, but that can happen in another PR.

I think the easiest solution is to not towork around it and wait for ipykernel 5.1.3 (or later) to be released.

Merging!

mdickinson added 12 commits September 5, 2019 11:34

First steps towards making IPKernelApp disposable.

515aaf1

More progress towards ipykernel cleanup.

504a23c

More cleanup

b48af73

More cleanup

96836b1

No more leaked objects

cbd12f7

Fix uncollectable garbage under Python 2.

5737c0f

Atexit fixes; attempted heartbeat fix (needs changes to the heartbeat…

7e82617

… code).

Custom heartbeat code

dfef826

Fix trait definition with hard-coded type.

34766d0

Add reminder to subclass Heartbeat so that we don't have to redeclare…

cefdd39

… the trait.

Cleanup of more globals.

eb202c3

Move the kernel application into its own module.

d73c516

mdickinson added 4 commits September 10, 2019 08:19

Tidy up heartbeat code; update comments.

52dcd00

Comment indicating when it's safe to remove this workaround.

bc67ca5

Fix docstring.

9853cc4

Remove some unnecessary bits; move some machinery from internal_ipker…

af8b4d8

…nel to kernelapp

jwiggins reviewed Sep 10, 2019

View reviewed changes

envisage/plugins/ipython_kernel/internal_ipkernel.py Show resolved Hide resolved

mdickinson added 5 commits September 10, 2019 11:04

Move some more of the shutdown / cleanup code to the IPKernelApp; rem…

d9b9685

…ove some unnecessary cleanup

More cleanup; but we're now leaking a shell on Python 3 and failing o…

7ca5721

…n Python 2.

Fix use of logging.hasHandlers on Python 2.7; move IPython.utils.io.s…

52a021b

…tdout caching to init_kernel.

Final(?) cleanups on internal_ipkernel.

5b385fd

Tweaks: remove outdated comment; fix grammar, spacing

cc1beae

mdickinson added 4 commits September 10, 2019 14:18

Add tests.

f30bff8

Add test for atexit handlers.

a641613

Expand comment for test_no_new_atexit_handlers to suggest strategies …

cf83351

…on test failure.

Add test for restoration of __main__ in sys.modules

23e40ce

Add missing Popen.wait() call to avoid 'ResourceWarning' messages.

be2533e

mdickinson commented Sep 11, 2019

View reviewed changes

envisage/plugins/ipython_kernel/internal_ipkernel.py Outdated Show resolved Hide resolved

Remove an unnecessary close call; fix a typo in a docstring.

c894fad

corranwebster reviewed Sep 11, 2019

View reviewed changes

envisage/plugins/ipython_kernel/heartbeat.py Show resolved Hide resolved

corranwebster reviewed Sep 11, 2019

View reviewed changes

envisage/plugins/ipython_kernel/heartbeat.py Show resolved Hide resolved

corranwebster reviewed Sep 11, 2019

View reviewed changes

envisage/plugins/ipython_kernel/internal_ipkernel.py Outdated Show resolved Hide resolved

corranwebster reviewed Sep 11, 2019

View reviewed changes

corranwebster approved these changes Sep 11, 2019

View reviewed changes

mdickinson added 5 commits September 12, 2019 09:50

Merge remote-tracking branch 'origin/master' into fix/ipkernel-shutdown

535fc1b

Don't delete sockets; some of the newer traitlets subclasses don't al…

be444ce

…low this.

Rename shutdown to close, remove atexit handler, terminate context.

c431734

Override log_connection_info.

2ae39d8

Now that we're overriding log_connection_info, no need to change the …

54bda86

…ctrl_c message.

mdickinson added 3 commits September 12, 2019 11:23

Remove all the machinery for monkey-patching raw_print, raw_print_err…

b6682ba

… and ctrl_c_message.

Style: suppress flake8 long line warning

fa6a89c

Also close the heartbeat socket in the case of unexpected errors.

ab5b258

mdickinson merged commit 548225e into master Sep 13, 2019

mdickinson deleted the fix/ipkernel-shutdown branch September 13, 2019 07:21

rahulporuri mentioned this pull request Aug 14, 2020

Remove ipykernel Heartbeat patch when updating to ipykernel version >5 #298

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix IPython kernel-related cleanup #188

Fix IPython kernel-related cleanup #188

mdickinson commented Sep 10, 2019 •

edited

Loading

codecov-io commented Sep 10, 2019 •

edited

Loading

mdickinson commented Sep 10, 2019

mdickinson commented Sep 10, 2019

rahulporuri commented Sep 11, 2019

corranwebster Sep 11, 2019

mdickinson Sep 12, 2019

mdickinson Sep 13, 2019

corranwebster left a comment

mdickinson commented Sep 12, 2019

mdickinson commented Sep 12, 2019

mdickinson commented Sep 12, 2019

mdickinson commented Sep 12, 2019

mdickinson commented Sep 13, 2019

Fix IPython kernel-related cleanup #188

Fix IPython kernel-related cleanup #188

Conversation

mdickinson commented Sep 10, 2019 • edited Loading

codecov-io commented Sep 10, 2019 • edited Loading

Codecov Report

mdickinson commented Sep 10, 2019

mdickinson commented Sep 10, 2019

rahulporuri commented Sep 11, 2019

corranwebster Sep 11, 2019

Choose a reason for hiding this comment

mdickinson Sep 12, 2019

Choose a reason for hiding this comment

mdickinson Sep 13, 2019

Choose a reason for hiding this comment

corranwebster left a comment

Choose a reason for hiding this comment

mdickinson commented Sep 12, 2019

mdickinson commented Sep 12, 2019

mdickinson commented Sep 12, 2019

mdickinson commented Sep 12, 2019

mdickinson commented Sep 13, 2019

mdickinson commented Sep 10, 2019 •

edited

Loading

codecov-io commented Sep 10, 2019 •

edited

Loading