-
-
Notifications
You must be signed in to change notification settings - Fork 373
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
|
@@ -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: t.Optional[Thread] = None | ||
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] | ||
|
||
|
@@ -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" | ||
# deprecated: | ||
"apply_request", | ||
] | ||
|
@@ -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 " | ||
"function in the future. Coroutine functions have been supported " | ||
"since ipykernel 6.0 (2021)", | ||
DeprecationWarning, | ||
stacklevel=2, | ||
) | ||
|
||
async def process_control(self): | ||
try: | ||
while True: | ||
|
@@ -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 `kernelbase.py` since ipykernel 6.10" | ||
" (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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not throwing the warning if There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. With a X.0 release I much prefer to be strict(er) on a API, and relax if needs be than the opposite. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 edit: fixed playground link There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 This also make forgetting an
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:
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 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" There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 ?
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.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
I can work on that, if you want, since I'm working on the ipyparallel updates now anyway There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"] | ||
|
@@ -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): | ||
|
@@ -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: | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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:
ipykernel/ipykernel/kernelbase.py
Lines 223 to 233 in 2ca7992
There was a problem hiding this comment.
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.