-
Notifications
You must be signed in to change notification settings - Fork 947
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
Add kernel.py file containing kernel-related routines #2272
Conversation
...containing get_kernel/register_target/display/clear_output
6e1c73d
to
f4c692b
Compare
from ._version import version_info, __version__, __protocol_version__, __jupyter_widgets_controls_version__, __jupyter_widgets_base_version__ | ||
from .widgets import * | ||
from traitlets import link, dlink | ||
|
||
|
||
def load_ipython_extension(ip): |
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.
I'm not sure about the changes on the __init__.py
, I didn't know what to keep, what to change, and what to remove.
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.
After in-person discussion with Sylvain, this doesn't seem to be useful. Maybe @jasongrout you have an idea? Do you think this can be removed? Or stay as an alias for register_comm_target
for backward compatibility?
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.
I like the title and comments and the description of the PR, and I'd really like to understand the issue, and the solution. Maybe a video call can help, or some more context. Happy to help with the PR though!
@maartenbreddels I edited the the PR description :) |
No worries. Starting to understand it better. I didn't understand it was for xeus_python, now it starts to make more sense. I do think the get_ipython and get_kernel is a bit confusing. If there is a good reason to do I think some comments may help. |
The thing is that |
024172c
to
589f8f4
Compare
589f8f4
to
b834d3d
Compare
I think we can close this PR. It is a bit outdated. And IMO we should not mock ipywidgets to have it working in xeus-python, as in ipywidgets should not be a special case. I made a PR using IPython in xeus-python: jupyter-xeus/xeus-python#400, which should fix most of these issues. We still need to mock |
We need those changes for
xeus_python
. We implemented widgets support, but it enforces us to mock IPython an ipykernel at different places:IPython.display.display
,IPython.display.clear_output
,IPython.get_ipython
,ipykernel.comm.Comm
. See https://github.com/QuantStack/xeus-python/blob/master/src/xinterpreter.cpp#L41.Those changes help by putting all those imports in one place:
kernel.py
. So that we just need to mockipywidgets.kernel
inxeus_python
.