-
-
Notifications
You must be signed in to change notification settings - Fork 388
Continue to support Kernel._parent_ident
for backward compatibility
#1427
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
base: main
Are you sure you want to change the base?
Conversation
I have confirmed locally on Linux that this fixes the problematic JupyterLab tests, which are: $ cd packages/services
$ jlpm test test/kernel/ikernel.spec.ts
<snip>
Test Suites: 1 passed, 1 total
Tests: 71 passed, 71 total
Snapshots: 0 total
Time: 118.164 s
Ran all test suites matching /test\/kernel\/ikernel.spec.ts/i.
|
@@ -313,6 +318,15 @@ def __init__(self, **kwargs): | |||
self._shell_parent_ident = ContextVar("shell_parent_ident") | |||
self._shell_parent_ident.set(b"") | |||
|
|||
# For backward compatibility so that _parent_ident["shell"] and _parent_ident["control"] | |||
# work as they used to for ipykernel >= 7 | |||
self._parent_ident = LazyDict( |
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.
Do we want to make this a property with a deprecation warning pointing to a stable public API to use instead?
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.
We could but as far as I can tell there isn't currently a public API for this and I don't personally want to extend the public API.
I think that ipykernel
8 is likely to support concurrent execution of async code cells (via anyio
or not), and the timeline for version 7 to 8 will be much faster than for 6 to 7. For that the concepts of a current parent header and ident won't exist so we'd either have to remove the caching of such information and pass it around as arguments, or at least make the caching more complex such than any public API we could invent now would have to be changed again. So allowing downstream libraries to continue to use _parent_ident
for the 7.x series seems sensible.
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.
makes sense, thanks
@@ -146,6 +146,12 @@ Submodules | |||
:show-inheritance: | |||
|
|||
|
|||
.. automodule:: ipykernel.utils |
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.
do we want LazyDict to be a publicly documented part of the API? I would think we'd want it to be a private implementation detail.
PR #1414 replaced the private member variable
Kernel._parent_ident
with threadsafe support of separate parent idents per thread (subshell) usingContextVar
s. This caused a failure downstream in JupyterLab (jupyterlab/jupyterlab#17785) as it reads the private_parent_ident
. Upon investigation there are a number of downstream projects that read this variable, presumably due to copy-paste-modify ofipykernel
code to override the behaviour. So rather than break those projects, here I am reintroducingKernel._parent_ident
as a shim for backward compatibility that can be called the same way as before. It uses a lazy-evaluated read-only dictionary.I have also added a new test to check that
Kernel.get_parent
andKernel._parent_ident
perform as expected.