Detect subclass overrides of cleanup methods

[kevin-bates]

Rather than use version comparisons to determine if the legacy cleanup() method should be used - which unnecessarily takes a deprecated route for applications that don't override cleanup() in subclasses or override KernelManager at all - this checks if an override of both cleanup() and cleanup_resources() exist. If only the former, the deprecated route is taken by calling cleanup(), otherwise cleanup_resources() is called.

Resolves #558

[MSeal]

Probably worth having a test to make sure the logic works for a subclass?

[kevin-bates]

I went back and forth on whether these tests should subclass the existing test classes and execute all the tests or just perform the general lifecycle test - focusing on which of the two cleanup methods was called. I settled on the latter - although it might help to know all kernel manager tests pass with either kind of subclass in use.

I also found that the override method checks needed to be added to AsyncKernelManager.shutdown_kernel() as well.

Some other observations:

• The tests could use refactoring and conversion to pytest. (Not sure when I'd have time to help with that.)
• A deprecation warning for cleanup will still be produced when MultiKernelManager.shutdown_all() occurs. Since that's on MKM, its a bit nastier, although things would probably just fall out if self.shutdown_kernel(kid) where called instead. Not sure why it's doing the current code.
• Seems like a mistake to expose request_shutdown(), finish_shutdown() and cleanup() at the MKM level given that KM.shutdown_kernel() makes those calls.

[kevin-bates]

I'm taking a crack at converting the kernel manager tests to pytest.

I found a bug in TestParallel. It's not setting up the transport correctly, so ipc is not being tested. I'm finding that once correct, test_start_parallel_process_kernels[ipc] is failing with the following...

test_kernelmanager.py::TestParallel::test_start_parallel_process_kernels[ipc] FAILED [ 75%]Process Process-2:
Traceback (most recent call last):
  File "/opt/anaconda3/envs/jupyter-client-dev/lib/python3.6/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/opt/anaconda3/envs/jupyter-client-dev/lib/python3.6/multiprocessing/process.py", line 93, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/kbates/repos/oss/jupyter/jupyter_client/jupyter_client/tests/test_kernelmanager.py", line 292, in _run_signaltest_lifecycle
    kc = self._prepare_kernel(km, stdout=PIPE, stderr=PIPE)
  File "/Users/kbates/repos/oss/jupyter/jupyter_client/jupyter_client/tests/test_kernelmanager.py", line 282, in _prepare_kernel
    kc.wait_for_ready(timeout=startup_timeout)
  File "/Users/kbates/repos/oss/jupyter/jupyter_client/jupyter_client/blocking/client.py", line 113, in wait_for_ready
    msg = self.iopub_channel.get_msg(block=True, timeout=0.2)
  File "/Users/kbates/repos/oss/jupyter/jupyter_client/jupyter_client/blocking/channels.py", line 55, in get_msg
    return self._recv()
  File "/Users/kbates/repos/oss/jupyter/jupyter_client/jupyter_client/blocking/channels.py", line 43, in _recv
    return self.session.deserialize(smsg)
  File "/Users/kbates/repos/oss/jupyter/jupyter_client/jupyter_client/session.py", line 934, in deserialize
    raise ValueError("Invalid Signature: %r" % signature)
ValueError: Invalid Signature: b'84517ae08515f7e239e1a73d6bef8344fda1e46d20c0dba2b8fbf132ab1b85e4'

[kevin-bates]

I've got the kernel tests converted to pytest (see last two commits here). I have added code to skip the two parallel tests (the thread version is also failing, so there's some intermittency there) when transport == ipc so as to not distract from the intention of this PR.

@MSeal - should I go ahead and include the test refactor in this PR or wait for this to be merged? (The test refactor is based on this PR branch.)

[MSeal]

I'm ok with either a separate PR or a build upon this one. Whichever is easiest for you. If you're indifferent, keep the PRs separate.

[MSeal]

Hmm, we may not want to write into their real jupyter_data_dir during a test. I would have maybe opted to just mock function call and check that the correct mock is called when a subclass with the old vs new pattern exists. If we do the full integration test pattern like this we likely want to use a temporary directory to cleanup the files automatically and not pollute the system. This might be a bit of a pain to do though :/

[kevin-bates]

This install method is used by several existing tests as well. It doesn't create the kernelspec in the real jupyter_data_dir because the environment setup method points the "important" envs to a temporary directory, knowing that those envs take precedence.

[MSeal]

Alright that works, thanks for clarifying

[MSeal]

• The tests could use refactoring and conversion to pytest. (Not sure when I'd have time to help with that.)
  Yeah there's a fair bit of that still needing addressing in jupyter libraries. I can probably find time later in the week to help if you want me to.

• A deprecation warning for cleanup will still be produced when MultiKernelManager.shutdown_all() occurs. Since that's on MKM, its a bit nastier, although things would probably just fall out if self.shutdown_kernel(kid) where called instead. Not sure why it's doing the current code.
• Seems like a mistake to expose request_shutdown(), finish_shutdown() and cleanup() at the MKM level given that KM.shutdown_kernel() makes those calls.

I agree. I _think _this was a left-over before some refactoring occurred? I'd be fine with changing it but I don't know the full implications.

[kevin-bates]

I'm fine with separate PRs. Per my comment within the review, I don't think there are any changes necessary after all. I'll submit the test refactor once this is merged then.

I agree. I _think _this was a left-over before some refactoring occurred? I'd be fine with changing it but I don't know the full implications.

I believe we should do the following...

• Perform a couple of passes across the KM instances, just as now, but something like the following...

    def shutdown_all(self, now=False):
        """Shutdown all kernels."""
        kids = self.list_kernel_ids()
        for kid in kids:
            km = self.get_kernel(kernel_id)
            km.shutdown_kernel(now=False)
        for kid in kids:
            km = self.get_kernel(kernel_id)
            if km.is_alive():
                km.shudown_kernel(now=True)
            self.remove_kernel(kid)

and the cleanup of the cache_ports would probably need to move into remove_kernel().

• It would be good to deprecate request_shutdown(), finish_shutdown(), cleanup() and cleanup_resources() from MultiKernelManager, although we probably need to check Notebook's usage here. I'm guessing that directly clients of jupyter_client tend to not use MKM, but that may be wrong.
  ((A quick check against Notebook shows no calls to the MKM versions of these shutdown methods.))

[kevin-bates]

For now, I've added a commit to avoid the deprecation warning stemming from MKM.shutdown_all() so the only warnings relative to calls to cleanup() should be those from the test itself (or any subclasses that implement cleanup() and not cleanup_resources() 😄 )

[MSeal]

Thanks so much for making that PR @kevin-bates !