Misc cleanup and types updates #1272
Conversation
ipykernel/kernelapp.py
Outdated
| iopub_thread: Thread | ||
| control_thread: Thread | ||
| shell_channel_thread: Thread |
There was a problem hiding this comment.
I'm not sure if it's overridden in subclasses somewhere, but sure I can try so if mypy ever complains.
| # 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" |
There was a problem hiding this comment.
Maybe not leave this in the codebase?
There was a problem hiding this comment.
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.
debug_request is not related to ipyparallel. ipyparallel doesn't use or support the debugging messages.
There was a problem hiding this comment.
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.
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.
| DeprecationWarning, | ||
| stacklevel=1, | ||
| ) | ||
| if inspect.isawaitable(reply_content): |
There was a problem hiding this comment.
Why not throwing the warning if reply_content is not awaitable?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think DeprecationWarning or PendingDeprecationWarning is fine, either way.
Co-authored-by: David Brochart <[email protected]>
| @@ -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.
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_replyapply_requestabort_requestclear_requestdo_clear
I can work on that, if you want, since I'm working on the ipyparallel updates now anyway
| 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.
Not deprecated. This is part of the debugger protocol, not part of IPython Parallel
Subset of ipython#1272 – which should be incontroversial and does not conflict with recent changes.
Extracted from ipython#1272
…ble. See discussion in ipython#1272; It is not deprected, but being able to always know you can (and must) await should be simpler in the long run. Deprecating now is not the point, but I want to cover our bases, so that we are more confident later when and if we want to enforce those await. In particular many of those branches are not covered in our tests – and I don't even know wether they were ever taken; I changed some of the base methods to be async, but I'm happy to move those back to sync. A few other things use the `if awaitable(...):` pattern but are a bit more complicted, and some do not dates from 2021, so those will be dealt with separately.
…ble. See discussion in ipython#1272; It is not deprected, but being able to always know you can (and must) await should be simpler in the long run. Deprecating now is not the point, but I want to cover our bases, so that we are more confident later when and if we want to enforce those await. In particular many of those branches are not covered in our tests – and I don't even know wether they were ever taken; I changed some of the base methods to be async, but I'm happy to move those back to sync. A few other things use the `if awaitable(...):` pattern but are a bit more complicted, and some do not dates from 2021, so those will be dealt with separately.
…ble. See discussion in ipython#1272; It is not deprecated, but being able to always know you can (and must) await should be simpler in the long run. Deprecating now is not the point, but I want to cover our bases, so that we are more confident later when and if we want to enforce those await. In particular many of those branches are not covered in our tests – and I don't even know wether they were ever taken; I changed some of the base methods to be async, but I'm happy to move those back to sync. A few other things use the `if awaitable(...):` pattern but are a bit more complicted, and some do not dates from 2021, so those will be dealt with separately.
…ble. See discussion in ipython#1272; It is not deprecated, but being able to always know you can (and must) await should be simpler in the long run. Deprecating now is not the point, but I want to cover our bases, so that we are more confident later when and if we want to enforce those await. In particular many of those branches are not covered in our tests – and I don't even know wether they were ever taken; I changed some of the base methods to be async, but I'm happy to move those back to sync. A few other things use the `if awaitable(...):` pattern but are a bit more complicted, and some do not dates from 2021, so those will be dealt with separately.
No description provided.