Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ianthomas23
Copy link
Collaborator

PR #1414 replaced the private member variable Kernel._parent_ident with threadsafe support of separate parent idents per thread (subshell) using ContextVars. 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 of ipykernel code to override the behaviour. So rather than break those projects, here I am reintroducing Kernel._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 and Kernel._parent_ident perform as expected.

@ianthomas23
Copy link
Collaborator Author

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(
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants