Minor Fixes. #1009
Minor Fixes. #1009
Conversation
I'm investigating an issue where Jupyter-Console does not and it seem there are mixup between sync and async KernelManager and other in a few places. Basically both Async and Blocking KM are expected by the type system to work as KM independently, but obviously some have Async methods and other have Sync methods, which need to be awaited. When touching traitlets config files it's easy to change the default class from Sync to async, and then you get weird error messages. So dropping a few asserts here and fixign a few types while investigating. I think the Sync and Async should be more distinct now that we are mostly async everywhere to avoid those confusions.
| @@ -311,6 +312,8 @@ def init_kernel_manager(self) -> None: | |||
| self.exit(1) # type:ignore[attr-defined] | |||
|
|
|||
| self.kernel_manager = t.cast(KernelManager, self.kernel_manager) | |||
|
|
|||
| assert not iscoroutinefunction(self.kernel_manager.start_kernel) | |||
There was a problem hiding this comment.
Is this for type checking? If not, using assert is a bad practice as they are removed when Python runs in optimized mode.
There was a problem hiding this comment.
Yes I know asserts are removed in optimized mode, but they are useful in non-optimized mode to debug broken invariant. That's the point of assert.
I was hitting a "non awaited coroutine" in a weird configuration, this make the failure explicit.
I don't know where this "using assert it bad because they are removed in optimized build", I mean that's the point of asserts right ? From the python docs
Assert statements are a convenient way to insert debugging assertions into a program
There was a problem hiding this comment.
But now you have code that behaves differently depending on a setting that you don't control.
You can replace it with a check and raise an exception with a meaningful description of the error.
There was a problem hiding this comment.
But now you have code that behaves differently depending on a setting that you don't control.
You can replace it with a check and raise an exception with a meaningful description of the error.
Yeah, but I don't want to have a permanent check for something that is never supposed to happen.
There was a problem hiding this comment.
But you have it for users who don't run in optimized mode.
|
Thanks for the review. I don't want to argue about things like this and I don't want to make it a real error. |
I'm investigating an issue where Jupyter-Console does not and it seem there are mixup between sync and async KernelManager and other in a few places.
Basically both Async and Blocking KM are expected by the type system to work as KM independently, but obviously some have Async methods and other have Sync methods, which need to be awaited.
When touching traitlets config files it's easy to change the default class from Sync to async, and then you get weird error messages.
So dropping a few asserts here and fixign a few types while investigating.
I think the Sync and Async should be more distinct now that we are mostly async everywhere to avoid those confusions.