Type checking #83

davidbrochart · 2020-06-19T09:20:00Z

No description provided.

choldgraf · 2020-06-19T14:55:46Z

This LGTM but I'm probably not the best to review since I am still a heathen that has never used Python type checking before :-)

davidbrochart · 2020-06-19T15:01:13Z

Same for me just before this PR 😄

MSeal

This is great and all the changes look good! Thanks so much for diving into it. I left some minor items to discuss on the PR.

MSeal · 2020-06-19T17:08:48Z

nbclient/client.py

@@ -51,7 +55,7 @@ class NotebookClient(LoggingConfigurable):
        ),
    ).tag(config=True)

-    timeout_func = Any(
+    timeout_func: t.Any = Any(


Usually you do from typing import Any, but this is fine

Yes but some names in typing collide with traitlets (List, Any, Dict), it's annoying.

Oh... yeah good point

MSeal · 2020-06-19T17:13:06Z

nbclient/client.py

+    def __init__(
+            self,
+            nb: NotebookNode,
+            km: t.Optional[KernelManager] = None,


Technically there's a KernelManagerABC.. but I don't think anyone directly inherits from the abc other than KernelManager. But it might be a good idea to change to over.

@kevin-bates FYI I think we should shift the RemoteKernelManager to inherit from the KernelManagerABC so we can have a consistent type check / interface. From looking at the class I believe this wouldn't disrupt anything therein.

If RemoteKernelManager inherits directly from KernelManagerABC instead of the concrete KernelManager, how does it then leverage the implementation of KernelManager?

RKM is not an independent implementation of KernelManagerABC - otherwise that's what it would inherit from. It needs to be plugged into the framework and that is accomplished by deriving from KernelManager.

I tracked down the inheritance and it ends up at MappingKernelManager->MultiKernelManager -- which means we should instead make MultiKernelManager inherit the KernelManagerABC -- though the contract for MKM uses some reserved kwargs that KM does not on it's methods -- for type hinting check purposes. Or we can make this type check a union of the two classes. Drifting from the topic at hand (can move elsewhere if need be) is there a historical reason the MappingKernelManager is in notebook over jupyter_client? Where do we draw the line on custom extensions within the jupyter github repos for kernel managers?

RKM is not an independent implementation of KernelManagerABC - otherwise that's what it would inherit from. It needs to be plugged into the framework and that is accomplished by deriving from KernelManager.

Think of KernelManagerABC as an interface mixin -- you can inherit from it and other things without issue using Python's default inheritance ordering for overlapping method names. Given the chain of existing inheritance it does reach into the jupyter_client as I pointed out above, so likely we need to just resolve that in jupyter_client without wanting/needing any changes in EG.

Let's merge for now and we can open new issues if there's problems with EG patterns in combination.

MSeal · 2020-06-19T17:27:21Z

nbclient/client.py

@@ -344,13 +354,13 @@ async def _async_cleanup_kernel(self):
        finally:
            # Remove any state left over even if we failed to stop the kernel
            await ensure_async(self.km.cleanup())
-            if getattr(self, "kc"):
+            if self.kc is not None:


I believe this one actually needs to be getattr(self, "kc") or getattr(self, "kc") is not None because the KernelGateway (and possibly other?) code paths sometimes don't assign any kc to the object by the time it reaches this call sometimes. I see you were testing this with asserts within the functions in places, but I can't quite recall the exact circumstances that cause the issue originally.

So I changed it because otherwise we get error: Item "None" of "Optional[Any]" has no attribute "stop_channels" with the following line. I think it should be:

if getattr(self, "kc") and self.kc is not None:

sounds good to me

MSeal · 2020-06-19T17:29:17Z

nbclient/client.py

@@ -419,7 +430,7 @@ def setup_kernel(self, **kwargs):

        # Can't use run_until_complete on an asynccontextmanager function :(
        if self.km is None:
-            self.start_kernel_manager()
+            self.km = self.start_kernel_manager()


Not really necessary to do the double assignment since self.start_kernel_manager() assigns self.km regardless, but doesn't really hurt anything either for protecting subclasses not implementing the same side-effect.

If we don't assign to self.km we get error: Item "None" of "Optional[Any]" has no attribute "has_kernel" with the following line.

MSeal · 2020-06-19T17:31:31Z

nbclient/client.py

+            msg_id: str,
+            cell: t.Optional[NotebookNode] = None) -> t.Optional[t.Dict]:
+
+        assert self.kc is not None


A little odd to keep the asserts in non-test code.

Here too, without the assert we get error: Item "None" of "Optional[Any]" has no attribute "shell_channel" later in the method, because the type checker has no idea self.kc has a non-None value at that time.

MSeal · 2020-06-19T17:34:12Z

nbclient/client.py

@@ -820,13 +879,13 @@ def output(self, outs, msg, display_id, cell_index):
            # default output behaviour (e.g. OutputWidget)
            hook = self.output_hook_stack[parent_msg_id][-1]
            hook.output(outs, msg, display_id, cell_index)
-            return
+            return None


Had to reread the PEP on this, but when returning elsewhere with a value it is recommended to have a return None explicitly where exiting early without an object. So good catch and I'll have to remember that in the future!

explicit is better than implicit! :-)

Yes, this is enforced buy the type checker that complains with error: Return value expected otherwise.

davidbrochart · 2020-06-19T21:05:07Z

nbclient/client.py

-    def _timeout_with_deadline(self, timeout, deadline):
-        if deadline is not None and deadline - monotonic() < timeout:
-            timeout = deadline - monotonic()
-
-        if timeout < 0:
-            timeout = 0
-
-        return timeout


Note that this method seemed to be never called, so I removed it.

Huh, wonder how that happened. Probably we missed nuking it in an earlier refactor. Good catch.

MSeal · 2020-06-22T16:47:24Z

Thanks @davidbrochart for knocking this out!

Type check nbclient.py

6c45e50

davidbrochart mentioned this pull request Jun 19, 2020

Type annotations #82

Closed

davidbrochart added 10 commits June 19, 2020 11:42

Fix linter

d35244c

Fix for test_deadline_exec_reply

0963355

Fix mypy

9a9ed99

Add mypy.ini

390392d

Include mypy.ini in MANIFEST.in

fc42084

Type check nbclient/util.py

2f6a820

More file checking

4569594

Fix circular import

93e4403

Fix linter

12df5b0

Explicitly list files for mypy

0977cb4

davidbrochart changed the title ~~[WIP] Type checking~~ Type checking Jun 19, 2020

MSeal suggested changes Jun 19, 2020

View reviewed changes

Changes according to review

5b93e2d

davidbrochart commented Jun 19, 2020

View reviewed changes

MSeal approved these changes Jun 22, 2020

View reviewed changes

MSeal merged commit 202e046 into jupyter:master Jun 22, 2020

davidbrochart deleted the type branch June 22, 2020 17:32

JoostvDoorn mentioned this pull request Jul 1, 2020

Fix usage of hasattr for kc #87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type checking #83

Type checking #83

davidbrochart commented Jun 19, 2020

choldgraf commented Jun 19, 2020

davidbrochart commented Jun 19, 2020

MSeal left a comment

MSeal Jun 19, 2020

davidbrochart Jun 19, 2020

MSeal Jun 19, 2020

MSeal Jun 19, 2020

MSeal Jun 19, 2020

kevin-bates Jun 19, 2020 •

edited

Loading

MSeal Jun 19, 2020 •

edited

Loading

MSeal Jun 22, 2020

MSeal Jun 19, 2020

davidbrochart Jun 19, 2020

MSeal Jun 19, 2020

MSeal Jun 19, 2020

davidbrochart Jun 19, 2020

MSeal Jun 19, 2020

davidbrochart Jun 19, 2020 •

edited

Loading

MSeal Jun 19, 2020

choldgraf Jun 19, 2020

davidbrochart Jun 19, 2020

davidbrochart Jun 19, 2020

MSeal Jun 19, 2020

MSeal commented Jun 22, 2020

Type checking #83

Type checking #83

Conversation

davidbrochart commented Jun 19, 2020

choldgraf commented Jun 19, 2020

davidbrochart commented Jun 19, 2020

MSeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevin-bates Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

MSeal Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbrochart Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSeal commented Jun 22, 2020

kevin-bates Jun 19, 2020 •

edited

Loading

MSeal Jun 19, 2020 •

edited

Loading

davidbrochart Jun 19, 2020 •

edited

Loading