Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misc cleanup and types updates #1272

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ipykernel/kernelapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from io import FileIO, TextIOWrapper
from logging import StreamHandler
from pathlib import Path
from threading import Thread

import zmq
import zmq.asyncio
Expand Down Expand Up @@ -142,9 +143,9 @@ class IPKernelApp(BaseIPythonApplication, InteractiveShellApp, ConnectionFileMix
debug_shell_socket = Any()
stdin_socket = Any()
iopub_socket = Any()
iopub_thread = Any()
control_thread = Any()
shell_channel_thread = Any()
iopub_thread: Thread
control_thread: Thread
shell_channel_thread: Thread
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's overridden in subclasses somewhere, but sure I can try so if mypy ever complains.


_ports = Dict()

Expand Down
73 changes: 60 additions & 13 deletions ipykernel/kernelbase.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import warnings
from datetime import datetime
from signal import SIGINT, SIGTERM, Signals
from threading import Thread

from .thread import CONTROL_THREAD_NAME

Expand Down Expand Up @@ -95,17 +96,16 @@ class Kernel(SingletonConfigurable):
implementation_version: str
banner: str

_is_test = Bool(False)
_is_test: bool = False

control_socket = Instance(zmq.asyncio.Socket, allow_none=True)
control_tasks: t.Any = List()

debug_shell_socket = Any()
control_thread: Thread
shell_channel_thread: Thread

control_thread = Any()
shell_channel_thread = Any()
iopub_socket = Any()
iopub_thread = Any()
stdin_socket = Any()
log: logging.Logger = Instance(logging.Logger, allow_none=True) # type:ignore[assignment]

Expand Down Expand Up @@ -217,6 +217,9 @@ def _parent_header(self):
"shutdown_request",
"is_complete_request",
"interrupt_request",
# I have the impression that there is amix/match debug_request and do_debug_request
# former was from ipyparallel but is marked as deprecated, but now call do_debug_request
# "do_debug_request"
Comment on lines +220 to +222
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not leave this in the codebase?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's an actual question I have, and I don't think it does belong in an issue.
I'm not too sure where to leave it, and I'm certain if I remove it, I will spend again an hour to figure that out next time I try to clean this up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug_request is not related to ipyparallel. ipyparallel doesn't use or support the debugging messages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then something in wrong on like 228 of the file (before modifications),
it does state that some of those are ipyparallel deprecated mesages:

# add deprecated ipyparallel control messages
control_msg_types = [
*msg_types,
"clear_request",
"abort_request",
"debug_request",
"usage_request",
"create_subshell_request",
"delete_subshell_request",
"list_subshell_request",
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the comment is outdated, it only applies to the first two message types here. From debug_request onward are control-channel-only messages. #1282 removes the deprecated messages and fixes the comment.

# deprecated:
"apply_request",
]
Expand Down Expand Up @@ -258,6 +261,17 @@ def __init__(self, **kwargs):
self.do_execute, ["cell_meta", "cell_id"]
)

if not inspect.iscoroutinefunction(self.do_debug_request):
# warning at init time, as we want to warn early enough, even if the
# function is not called
warnings.warn(
"`do_debug_request` will be required to be a coroutine "
"functions in the future. coroutine functions have ben supported "
Carreau marked this conversation as resolved.
Show resolved Hide resolved
"since ipykernel 6.0 (2021)",
DeprecationWarning,
stacklevel=2,
)

async def process_control(self):
try:
while True:
Expand Down Expand Up @@ -1007,13 +1021,35 @@ def do_is_complete(self, code):
return {"status": "unknown"}

async def debug_request(self, socket, ident, parent):
"""Handle a debug request."""
"""Handle a debug request.

Seem Deprecated
"""
# If this is incorrect, remove `debug_request` from the deprecated message types
# at the beginning of this class
warnings.warn(
"debug_request is deprecated in kernel_base since ipykernel 6.10"
Carreau marked this conversation as resolved.
Show resolved Hide resolved
" (2022). It is only part of IPython parallel. Did you mean do_debug_request ?",
DeprecationWarning,
stacklevel=2,
)
if not self.session:
return
content = parent["content"]
reply_content = self.do_debug_request(content)
if inspect.isawaitable(reply_content):
reply_content = await reply_content
if not inspect.iscoroutinefunction(self.do_debug_request):
# repeat the warning at run
reply_content = self.do_debug_request(content)
warnings.warn(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not deprecated. This is part of the debugger protocol, not part of IPython Parallel

"`do_debug_request` will be required to be a coroutine "
"functions in the future. coroutine functions have ben supported "
"since ipykernel 6.0 (2021)",
DeprecationWarning,
stacklevel=1,
)
if inspect.isawaitable(reply_content):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not throwing the warning if reply_content is not awaitable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because testing for awaitable is/was the incorrect things to do.

I believe at some point sync functions were returning Futures, which are awaitable.

We do not want the return a do_debug_request to be an awaitble; we want do_debug_request to be a coroutine function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that really a requirement? Why be unnecessarily restrictive like this when the two are used the same?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this makes it hard to find errors with static type checking, and other tools.
Seeing the version of Python we support their is no reasons to not use async/await anymore, and every code branch we support is some complexity we have to support.

With a X.0 release I much prefer to be strict(er) on a API, and relax if needs be than the opposite.

Copy link
Member

@minrk minrk Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are good reasons to return an awaitable rather than using a coroutine (it can have dramatic performance benefits), and cutting that off due to deficiencies in tooling does not seem like a positive change to me.

mypy appears to understand inspect.isawaitable since 1.11, so this check is correctly handled for type narrowing, but we can also use isinstance(thing, typing.Awaitable), which is equivalent and I think has worked in mypy for longer.

edit: fixed playground link

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do_xyz methods were sync-only for a long time, so most implementations are sync because they have no need to be async. It costs us nothing to keep it that I can see, and it breaks users for no benefit, so I don't understand the trade off. In general, I think you need a really good reason to break APIs, and version numbers don't really have any effect on that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It costs us nothing to keep it that I can see, and it breaks users for no benefit, so
I don't understand the trade off

It never "cost nothing" to keep optional things like this – it's not the first time you've used this justification, I used to agree, but I've grown.

It cost not much, but on a really long tail – and it accumulates. Keeping it means that every other consumer present or future of do_debug_request must keep in mind that it might be sync or async, and test for it – meaning that await do_debug_request(...) is technically wrong at it may be sync and consumer then must check for isawaitable(result) and await instead.

This also make forgetting an async in subclass a subtly incorrect but working implementations:

class Sup:

    async def do_debug(self):
        return 'res'

class Derived(Sup):

    def do_debug(self):
        print('Log')
        return super().do_debug()

Well you see the kind of issues.

The non-awaitable/non async path is BTW not tested, and the test in this repo assume the method is always a coroutine:

tests/test_ipkernel_direct.py
208:async def test_do_debug_request(ipkernel: IPythonKernel) -> None:
211:    await ipkernel.do_debug_request(msg)


tests/conftest.py
106:    async def do_debug_request(self, msg):

Personally I got bitten too many time by "it costs nothing" (think shim modules of IPython, and ipython_genutils and alike from a decade ago), that are still regularly used in client projects giving me headache because we should have ripped them out earlier instead of other folks (or us in the future) paying costs for a long time.

Same things with the it can be X or Y "for convenience" (Api with strings or bytes, $element or iterable of $element where a consumer ends up passing $element which is iterable and tnigs break,... etc) where you always end up chasing a bug for hours and cursing the API.

Hey up until last week I was chasing a bug in reformatting code in darker, because In IPython we have a test case in IPython for cyrillic, due to some lingering py3compat.cast_unicode in IPython ...

And again, I'm not asking to break now; I'm suggesting we tell consumer "hey this is a small but long term cost we will likely not want to incur forever"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with your assessment of the trade-offs in this case and think it shifts more cost to users than is appropriate, but since you feel strongly, if you want to keep the deprecation message for if not isawaitable(result) I can accept that.

I do super appreciate your diligence in adding "since when" info to deprecation messages. That has been a huge help for me.

Once all the deprecations about message handler functions are removed (the actually deprecated messages are now gone, debug_request is part of the standard kernel protocol, not deprecated or related to ipyparallel), this should be all set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with your assessment of the trade-offs in this case and think it shifts more cost to users than is appropriate, but since you feel strongly, if you want to keep the deprecation message for if not isawaitable(result) I can accept that.

Would a PendingDeprecationWarning, or a FutureWarning with a softer message ease your concerns ? saying that we are considering making it mandatory, and that we would prefer implementer to always return an awaitable ?

I do super appreciate your diligence in adding "since when" info to deprecation messages. That has been a huge help for me.

Thanks, it feel so frustrating when message don't give you this information and just give you the stick with "this will be removed in..."; Maybe one day I'll give a talk on why/how you should do that and other things in deprecation warnings and how to make it easier to push users for new API; or maybe just an blog post ? idk.

Once all the deprecations about message handler functions are removed (the actually deprecated messages are now gone, debug_request is part of the standard kernel protocol, not deprecated or related to ipyparallel), this should be all set.

I need to redo this PR anyway, some of the deprecation were due to improper comments and documentation; I'll try to get things in a more correct state; and maybe split that into multiple PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think DeprecationWarning or PendingDeprecationWarning is fine, either way.

reply_content = await reply_content
else:
reply_content = await self.do_debug_request(content)
reply_content = json_clean(reply_content)
reply_msg = self.session.send(socket, "debug_reply", reply_content, parent, ident)
self.log.debug("%s", reply_msg)
Expand Down Expand Up @@ -1132,7 +1168,12 @@ async def list_subshell_request(self, socket, ident, parent) -> None:

async def apply_request(self, socket, ident, parent): # pragma: no cover
"""Handle an apply request."""
self.log.warning("apply_request is deprecated in kernel_base, moving to ipyparallel.")
warnings.warn(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the async changes totally break all IPython Parallel versions that didn't have these methods, it probably makes sense to remove the IPython Parallel methods in ipykernel 7, as keeping them while breaking them at the same time doesn't make a lot of sense.

Methods/attributes to remove:

  • aborted
  • _send_abort_reply
  • apply_request
  • abort_request
  • clear_request
  • do_clear

I can work on that, if you want, since I'm working on the ipyparallel updates now anyway

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in #1282

"apply_request is deprecated in kernel_base since"
" IPykernel 6.10 (2022) , moving to ipyparallel.",
DeprecationWarning,
stacklevel=2,
)
try:
content = parent["content"]
bufs = parent["buffers"]
Expand Down Expand Up @@ -1174,8 +1215,11 @@ def do_apply(self, content, bufs, msg_id, reply_metadata):

async def abort_request(self, socket, ident, parent): # pragma: no cover
"""abort a specific msg by id"""
self.log.warning(
"abort_request is deprecated in kernel_base. It is only part of IPython parallel"
warnings.warn(
"abort_request is deprecated in kernel_base since ipykernel 6.10"
" (2022). It is only part of IPython parallel",
DeprecationWarning,
stacklevel=2,
)
msg_ids = parent["content"].get("msg_ids", None)
if isinstance(msg_ids, str):
Expand All @@ -1193,8 +1237,11 @@ async def abort_request(self, socket, ident, parent): # pragma: no cover

async def clear_request(self, socket, idents, parent): # pragma: no cover
"""Clear our namespace."""
self.log.warning(
"clear_request is deprecated in kernel_base. It is only part of IPython parallel"
warnings.warn(
"clear_request is deprecated in kernel_base since IPykernel 6.10"
" (2022). It is only part of IPython parallel",
DeprecationWarning,
stacklevel=2,
)
content = self.do_clear()
if self.session:
Expand Down
Loading