Add tests to ensure MultiKernelManager subclass methods are called #627

kevin-bates · 2021-03-17T00:37:38Z

This is a follow-on to #626 to check subclass call sequences relative to the MultiKernelManager base classes.

Also found that CI was skipping the async tests of the recently added TestKernelManagerShutDownGracefully resulting in these warnings:

jupyter_client/tests/test_kernelmanager.py::TestKernelManagerShutDownGracefully::test_async_signal_kernel_subprocesses[signaltest-_install_kernel-_ShutdownStatus.ShutdownRequest]
jupyter_client/tests/test_kernelmanager.py::TestKernelManagerShutDownGracefully::test_async_signal_kernel_subprocesses[signaltest-no-shutdown-install_kernel_dont_shutdown-_ShutdownStatus.SigtermRequest]
jupyter_client/tests/test_kernelmanager.py::TestKernelManagerShutDownGracefully::test_async_signal_kernel_subprocesses[signaltest-no-terminate-install_kernel_dont_terminate-_ShutdownStatus.SigkillRequest]
  /Users/runner/hostedtoolcache/Python/3.7.10/x64/lib/python3.7/site-packages/_pytest/python.py:172: PytestUnhandledCoroutineWarning: async def functions are not natively supported and have been skipped.
  You need to install a suitable plugin for your async framework, for example:
    - anyio
    - pytest-asyncio
    - pytest-tornasync
    - pytest-trio
    - pytest-twisted
    warnings.warn(PytestUnhandledCoroutineWarning(msg.format(nodeid)))

which was addressed by adding the @pytest.mark.asyncio decorator.

davidbrochart · 2021-03-17T08:23:03Z

jupyter_client/tests/utils.py

+    async def start_kernel(self, kernel_name=None, **kwargs):
+        self.record('start_kernel')
+        return await super().start_kernel(kernel_name=kernel_name, **kwargs)


It looks like we could have a common base class for SyncMKMSubclass and AsyncMKMSubclass, rather than duplicating all the methods. All the async methods can be regular functions, they will just return a result (sync) or an awaitable (async).
You could also use a mkm_method decorator as in:

jupyter_client/jupyter_client/multikernelmanager.py

Lines 27 to 40 in 26a16c0

def kernel_method(f):

"""decorator for proxying MKM.method(kernel_id) to individual KMs by ID"""

def wrapped(self, kernel_id, *args, **kwargs):

# get the kernel

km = self.get_kernel(kernel_id)

method = getattr(km, f.__name__)

# call the kernel's method

r = method(*args, **kwargs)

# last thing, call anything defined in the actual class method

# such as logging messages

f(self, kernel_id, *args, **kwargs)

# return the method result

return r

return wrapped

Thanks @davidbrochart. I'm having trouble visualizing the common base class approach that applies to both sync/async subclasses, could you please give an example?

Regarding the decorator, I'm not very familiar with this kind of thing, would it resemble something like this:

def mkm_recorder(f): def wrapped(self, *args, **kwargs): # record this call self.record(f.__name__) method = getattr(super(self.__class__, self), f.__name__) # call the superclass method r = method(*args, **kwargs) return r return wrapped class AsyncMKMSubclass(RecordCallMixin, AsyncMultiKernelManager): @mkm_recorder async def start_kernel(self, kernel_name=None, **kwargs): """ Record call and defer to superclass """ class SyncMKMSubclass(RecordCallMixin, MultiKernelManager): @mkm_recorder def start_kernel(self, kernel_name=None, **kwargs): """ Record call and defer to superclass """

Thanks.

I think something like that will do:

class MKMSubclass(RecordCallMixin): @mkm_recorder def get_kernel(self, kernel_id): """ Record call and defer to superclass """ # add all the other common methods here class SyncMKMSubclass(MKMSubclass, MultiKernelManager): def _kernel_manager_class_default(self): return 'jupyter_client.tests.utils.SyncKernelManagerSubclass' class AsyncMKMSubclass(MKMSubclass, AsyncMultiKernelManager): def _kernel_manager_class_default(self): return 'jupyter_client.tests.utils.AsyncKernelManagerSubclass'

Regarding the decorator, this looks fine.

kevin-bates · 2021-03-17T20:43:42Z

Thanks @davidbrochart - I've pushed an update that implements a decorator. I'd be interested in further refactoring so long as it doesn't affect the goal of ensuring a subclass's methods are called. Hmm, now that I think about it, would this be something accomplished with something like a subclass mixin class?

class SyncMKMSubclass(RecordCallMixin, MKMSubclassMixin, MultiKernelManager):
    def _kernel_manager_class_default(self):
        return 'jupyter_client.tests.utils.SyncKMSubclass'


class AsyncMKMSubclass(RecordCallMixin, MKMSubclassMixin, AsyncMultiKernelManager):
    def _kernel_manager_class_default(self):
        return 'jupyter_client.tests.utils.AsyncKMSubclass'

I'll experiment a bit on that note. Thanks for the ideas!

Update: It looks like I'd have to undo the decorator stuff to have the mixin approach work. Since the current approach better-adheres to the respective signatures of the sync/async forms, I think stopping here is good - unless you share some of your magic on this. 😄

davidbrochart · 2021-03-18T09:09:06Z

You can have a look at #628.

kevin-bates · 2021-03-18T14:07:22Z

Thank @davidbrochart. Ah - the trick is to use the private attribute _km_class. I had all of those changes in place with the exception of the attribute trick. I'll merge #628 and update this PR. Your changes will have some naming changes applied when merged with this PR.

davidbrochart · 2021-03-18T14:15:30Z

the trick is to use the private attribute _km_class.

Yes it was easier to have the super class explicitly named instead of trying to fit into an implicit mechanism, so I took a shortcut here 😄

kevin-bates · 2021-03-18T19:25:54Z

Looks like the CI failures are the result of ipykernel 5.5.1. They literally started occurring as soon as that release was out since my private fork was successful with these changes but happened to pull ipykernel 5.5.0.

jupyter_client/tests/test_client.py::TestKernelClient::test_execute_interactive is now failing on all 3.6 platforms with the following traceback:

jupyter_client/blocking/client.py:145: TimeoutError
----------------------------- Captured stderr call -----------------------------
[IPKernelApp] ERROR | Exception in message handler:
Traceback (most recent call last):
  File "/Users/runner/hostedtoolcache/Python/3.6.13/x64/lib/python3.6/site-packages/ipykernel/kernelbase.py", line 261, in dispatch_shell
    yield gen.maybe_future(handler(stream, idents, msg))
  File "/Users/runner/hostedtoolcache/Python/3.6.13/x64/lib/python3.6/site-packages/tornado/gen.py", line 762, in run
    value = future.result()
  File "/Users/runner/hostedtoolcache/Python/3.6.13/x64/lib/python3.6/site-packages/tornado/gen.py", line 769, in run
    yielded = self.gen.throw(*exc_info)  # type: ignore
  File "/Users/runner/hostedtoolcache/Python/3.6.13/x64/lib/python3.6/site-packages/ipykernel/kernelbase.py", line 538, in execute_request
    user_expressions, allow_stdin,
  File "/Users/runner/hostedtoolcache/Python/3.6.13/x64/lib/python3.6/site-packages/tornado/gen.py", line 762, in run
    value = future.result()
  File "/Users/runner/hostedtoolcache/Python/3.6.13/x64/lib/python3.6/site-packages/tornado/gen.py", line 234, in wrapper
    yielded = ctx_run(next, result)
  File "/Users/runner/hostedtoolcache/Python/3.6.13/x64/lib/python3.6/site-packages/tornado/gen.py", line 162, in _fake_ctx_run
    return f(*args, **kw)
  File "/Users/runner/hostedtoolcache/Python/3.6.13/x64/lib/python3.6/site-packages/ipykernel/ipkernel.py", line 290, in do_execute
    and should_run_async(code, transformed_cell=transformed_cell, preprocessing_exc_tuple=preprocessing_exc_tuple)
TypeError: should_run_async() got an unexpected keyword argument 'transformed_cell'

Since these failures are unrelated to this PR, I believe we can proceed with this PR's review.

blink1073 · 2021-03-18T19:41:47Z

I deleted ipykernel 5.5.1, cf ipython/ipykernel#607

davidbrochart · 2021-03-18T19:49:48Z

I don't want to be nitpicking, but TestKernelManager and TestAsyncKernelManager are very similar, and that's a lot of duplicated code. I think it is possible to have only one source of (async) tests, and have sync/async tests use this common base. For instance, you can await a non-async function by wrapping it with ensure_async:

jupyter_client/jupyter_client/util.py

Lines 22 to 25 in 92cb460

    
           async def ensure_async(obj): 
        
               if inspect.isawaitable(obj): 
        
                   return await obj 
        
               return obj

kevin-bates · 2021-03-18T20:25:49Z

@blink1073 - thank you. I've restarted the jobs.

@davidbrochart - no worries on the 'nitpicking' comment - I agree (with the duplication and don't believe you're being nitpicky 😄 ). However, I'd prefer that refactor be handled outside the scope of this pull request since it isn't necessarily related and has existed for some time (besides the fact that I don't have the bandwidth to deal with that at the moment).

Perhaps you could open an issue and someone can contribute those changes?

davidbrochart · 2021-03-18T20:42:46Z

No worries Kevin. You're right, we can refactor in a separate PR. Thanks!

kevin-bates added 2 commits March 16, 2021 16:51

Add tests to ensure MultiKernelManager subclass methods are called

60209a5

Fix TestKernelManagerShutDownGracefully async test

2786590

davidbrochart reviewed Mar 17, 2021

View reviewed changes

Use decorator for deferral to superclass, rename subclasses

a2c796c

kevin-bates mentioned this pull request Mar 18, 2021

Make KernelManager subclass tests DRY #628

Merged

Merge PR jupyter#628 into mkm-subclass-testing

ae9f6cc

davidbrochart merged commit 50dff2e into jupyter:master Mar 18, 2021

kevin-bates deleted the mkm-subclass-testing branch March 18, 2021 20:49

davidbrochart mentioned this pull request Mar 19, 2021

Add type annotations, refactor sync/async #623

Merged

blink1073 added the maintenance label Jul 30, 2021

blink1073 added this to the 7.0 milestone Jul 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests to ensure MultiKernelManager subclass methods are called #627

Add tests to ensure MultiKernelManager subclass methods are called #627

kevin-bates commented Mar 17, 2021

davidbrochart Mar 17, 2021

kevin-bates Mar 17, 2021 •

edited

Loading

davidbrochart Mar 17, 2021

kevin-bates commented Mar 17, 2021 •

edited

Loading

davidbrochart commented Mar 18, 2021

kevin-bates commented Mar 18, 2021

davidbrochart commented Mar 18, 2021

kevin-bates commented Mar 18, 2021

blink1073 commented Mar 18, 2021

davidbrochart commented Mar 18, 2021

kevin-bates commented Mar 18, 2021

davidbrochart commented Mar 18, 2021

	def kernel_method(f):
	"""decorator for proxying MKM.method(kernel_id) to individual KMs by ID"""
	def wrapped(self, kernel_id, args, *kwargs):
	# get the kernel
	km = self.get_kernel(kernel_id)
	method = getattr(km, f.__name__)
	# call the kernel's method
	r = method(args, *kwargs)
	# last thing, call anything defined in the actual class method
	# such as logging messages
	f(self, kernel_id, args, *kwargs)
	# return the method result
	return r
	return wrapped

Add tests to ensure MultiKernelManager subclass methods are called #627

Add tests to ensure MultiKernelManager subclass methods are called #627

Conversation

kevin-bates commented Mar 17, 2021

davidbrochart Mar 17, 2021

Choose a reason for hiding this comment

kevin-bates Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

davidbrochart Mar 17, 2021

Choose a reason for hiding this comment

kevin-bates commented Mar 17, 2021 • edited Loading

davidbrochart commented Mar 18, 2021

kevin-bates commented Mar 18, 2021

davidbrochart commented Mar 18, 2021

kevin-bates commented Mar 18, 2021

blink1073 commented Mar 18, 2021

davidbrochart commented Mar 18, 2021

kevin-bates commented Mar 18, 2021

davidbrochart commented Mar 18, 2021

kevin-bates Mar 17, 2021 •

edited

Loading

kevin-bates commented Mar 17, 2021 •

edited

Loading