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

make debugger class configurable #1307

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

smacke
Copy link

@smacke smacke commented Dec 13, 2024

In this PR, add fields compiler_class and debugger_class defaulting to the normal types. Kernel subclasses can override these to provide additional customizations.

@@ -110,11 +113,9 @@ def __init__(self, **kwargs):

self.executing_blocking_code_in_main_shell = False

from .debugger import Debugger, _is_debugpy_available
Copy link
Member

Choose a reason for hiding this comment

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

It looks like #1223 intentionally introduced this lazy import. Let's make sure we're addressing the need there as well before making this an eager import at the top of the file.

Copy link
Member

Choose a reason for hiding this comment

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

In particular, does this change mean that python -m ipykernel install --sys-prefix has spurious warnings?

Copy link
Member

@jasongrout jasongrout Dec 13, 2024

Choose a reason for hiding this comment

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

CC @krassowski who made this lazy import change earlier this year.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for spotting, yes this is intentional to address #1198

@@ -71,6 +72,8 @@ class IPythonKernel(KernelBase):
shell = Instance("IPython.core.interactiveshell.InteractiveShellABC", allow_none=True)
shell_class = Type(ZMQInteractiveShell)

debugger_class = Type(Debugger)
Copy link
Member

@jasongrout jasongrout Dec 13, 2024

Choose a reason for hiding this comment

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

You can also give this class as a full-specified dotted string (like in the shell instance above), so we could move the import above back to being an on-demand import in the class definition.

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 this should work:

Suggested change
debugger_class = Type(Debugger)
debugger_class = Type("ipykernel.debugger.Debugger")

Copy link
Author

@smacke smacke Dec 14, 2024

Choose a reason for hiding this comment

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

Thanks for the suggestion; I think if I do this I can omit the import altogether?

EDIT: for Debugger, but not for _is_debugpy_available I guess

Copy link
Member

@jasongrout jasongrout left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! In full disclosure, we'd like this at Databricks (where Stephen and I both work) because we customize the debugger class.

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

It seems fine, but not 100% sure as I get an error when trying to install kernel: #1309

@jasongrout
Copy link
Member

It seems fine, but not 100% sure as I get an error when trying to install kernel: #1309

Following up here for completeness, it seems like David fixes this error in #1310

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.

4 participants