search

jupyterlab / frontends-team-compass

A repository for team interaction, syncing, and handling meeting notes across the JupyterLab ecosystem.
https://jupyterlab-team-compass.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
59 stars 30 forks source link

Merge @jupyterlab/debugger into JupyterLab core #75

Closed afshin closed 4 years ago

afshin commented 4 years ago

We propose merging @jupyterlab/debugger into JupyterLab core as a 3.0 feature that only appears for users who have a kernel that supports debugging. Because the end-user experience is transparent, this feature should not cause disruption.

As of July 2020, the only kernel that supports the Debug Adapter Protocol is the xeus-python kernel, but we are eager for other kernels to support debugging. The front-end debugger is kernel-agnostic and will only show up for users who have installed at least one kernel that supports DAP.

cf. VSCode support for debugging Jupyter notebooks:

blink1073 commented 4 years ago

I support this proposal

ellisonbg commented 4 years ago

I support

On Wed, Jul 15, 2020 at 10:15 AM Steven Silvester notifications@github.com wrote:

I support this proposal

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jupyterlab/team-compass/issues/75#issuecomment-658892380, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAGXUHCOBMHOUS5LL3V5PTR3XP35ANCNFSM4O2XB7HQ .

-- Brian E. Granger

Principal Technical Program Manager, AWS AI Platform (brgrange@amazon.com) On Leave - Professor of Physics and Data Science, Cal Poly @ellisonbg on GitHub

saulshanabrook commented 4 years ago

I can understand the pressure to include this in core, especially since VS Code does now. And I am very thankful to all the work put in from folks to push this feature along, it will obviously have huge benefits to users.

However, I wanted to articulate one potential objection.

If we merge this in and release a 3.0 blog post announcing this feature, then users of JupyterLab will be presented with a choice:

  1. Run JupyterLab by default, and run notebooks, don't get access to new debugger feature.
  2. Install xeus-kernel after installing jupyterlab, get access to debugger but not to magics or IPython runtime library (I believe)

This will lead, I believe, to some pressure from users to move the community to a place where they can have a default kernel that will both have backwards compatible support and the debugger support.

So that means at some point in the future, if we merge this PR, we will either want to:

  1. Add more features to xeus-python and make it a default install with JupyterLab, switching the default kernel
  2. Add debugging support for ipykernel.

When we (at Quansight, including @Carreau) explored that question for a client of ours, we tentatively came to the conclusion that we should support bring xeus-python up to ipykernel support, because no one really wanted to keep maintaining ipykernel and adding debugger support would require a large rework.

But it required more funding than we had to even explore that question in a comprehensive way. If anyone has already done this exploration, it would be really helpful to share any findings from that to understand how much money and time that would require.

So my preference at the moment would be to wait on including debugger support until our default kernel we include has support for it.

If we include it, since we are a high visibility project, we will effectively have to announce it. And it's unclear what win including it brings, if we aren't advertising it as a new feature. Of course, it's more troublesome to install a third party extension, but that is true for many other extensions as well which have higher usage than this one.

Carreau commented 4 years ago

It's still unclear how much work is needed in ipykernel to get it to the right point, there are also a couple of things in the current jupyter implementation protocol that are incorrect/impossible, for example the control channel message receiving is blocked by the GIL, so it would need to be moved to a thread which has its own implications – xeus does that. It for example affects killing and restarting the kernel and there is hack in the notebook server that kill the process instead of sending a graceful shutdown message.

Also AFAICT after talking with Sylvain is that Xeus will not (and cannot) be 100% compatible with IPykernel (WRT configuration & extensions), and is not yet packaged on all platforms because it has to be compiled; So we will likely always have a need for an ipykernel. I've worked with sylvain to make the IPython utils better delineated so that xeux en

If core of JupyterLab want to merge support that's your decision, but I do think this may fracture the ecosystem, and that kernels maintainer (me), are stating to get tired of being single maintainer of projects no one want to spend critical time on (IPython, IPykernel, Traitlets). I did work with Sylvain to make IPython more modular and importable from Xeus, but I'm at the limit to the number of support request I can handle.

The client @saulshanabrook talked about really want an official way of using a debugger, but they do have a number of extensions/magics, and having their user switch kernels depending on the features supported will not make them happy.

There have already been a big fracture on the Python ecosystem with the move to JupyterLab, as people are still using classic notebook, and there is little time spent on it. So I would be mindful.

blink1073 commented 4 years ago

I am currently maintaining ipykernel, and will continue to do so. Members of the JupyterLab team are actively working to ease the transition between classic notebook and JupyterLab, and have always done our level best on that front.

Other kernel authors are working on adding debugger support. I don't think it is fair to say that we are causing a rift by offering a feature that is standard in IDEs.

afshin commented 4 years ago

The way I think about rolling out new features is based on these simple heuristics:

  1. Does this benefit our end users? A feature that is totally invisible and inert unless it can benefit an end user's experience after they already have a debugger-friendly kernel seems only good for users. If we are concerned about advertising it in a confusing way, I'd propose: let's not advertise it in a confusing way.
  2. Does this benefit our ecosystem partners / extension authors? The answer to this seems clear-cut too, insofar as it is a no-op.

If our own tools are shipping in other products, I submit they should ship with ours as well.

SylvainCorlay commented 4 years ago

Quick comments on xeus:

lresende commented 4 years ago 
* xeus-python supports IPython magics as of 0.8.2. The matplotlib magic is not yet supported.

I still see the magic issue open - https://github.com/jupyter-xeus/xeus-python/issues/63

* it is packaged for Linux, OSX, and Windows on conda. I have not yet pushed PyPI wheels for windows over questions with respect to the static build of openssl. It is a matter of a couple of days to sort that question out.

So, this means it has a dependency on anaconda? Or is pip install also supported? When I looked at the last year Python Developer Survey conda was used by only about 23% of python developers.

SylvainCorlay commented 4 years ago

I still see the magic issue open - jupyter-xeus/xeus-python#63

Cool.

So, this means it has a dependency on anaconda?

No. Why would you think so?

Or is pip install also supported? When I looked at the last year Python Developer Survey conda was used by only about 23% of python developers.

Cool. Pip install is supported on osx and linux. We build windows wheels as well in our CI but as I said, I have not uploaded them yet.

SylvainCorlay commented 4 years ago

Does this benefit our end users? A feature that is totally invisible and inert unless it can benefit an end user's experience after they already have a debugger-friendly kernel seems only good for users.

:+1: I agree and I support the proposal.

timkpaine commented 4 years ago

Does xeus have this critical feature of ipykernel?

from ipykernel import IPKernelApp; IPKernelApp.launch_instance()

Without the ability to launch from an existing python process it doesn't have much utility for those of us with large, custom python deployments.

Also the idea of having certain python kernels support certain features and not others seems bad. If you try to install non "big 3" kernels the user experience degrades substantially and it becomes very confusing very quick.

SylvainCorlay commented 4 years ago

Does xeus have this critical feature of ipykernel?

from ipykernel import IPKernelApp; IPKernelApp.launch_instance()

Not quite. You are free to use ipykernel if xeus-python does not fit your need.

Note that this issue is about incorporating the debugger front-end in lab. The debugging protocol was devised in a language agnostic fashion and other kernel authors are already looking at supporting it in their implementation.

timkpaine commented 4 years ago

Would it be possible to leave it out of the assets so as to not bloat the build with something we cannot use? If it's not going to work for most cases it should probably stay separate.

SylvainCorlay commented 4 years ago

Would it be possible to leave it out of the assets so as to not bloat the build with something we cannot use? If it's not going to work for most cases it should probably stay separate.

You can certainly use the debugger if you install a kernel that support debugging.

timkpaine commented 4 years ago

But if i don't install such a kernel because it doesn't work due to missing features, I'd like to not have the debugger assets included in my build.

Basically if it's included in the main build, it should work with the standard environment. If it requires extra things or customization, it should probably stay separate (I think resource utilization breaks this since nbresuse is required).

SylvainCorlay commented 4 years ago

I think that the inclusion of the front-end in the main bundle will

FYI we are currently also exploring the support of debugging in ipykernel. Xeus was primarily developed to make other kernels (R, C++, SQLite, and others), and going with xeus simplified things, but we are still very invested in the ipykernel project.

timkpaine commented 4 years ago

I just think more divergent kernels is a bad thing. Having to maintain a matrix of which python kernels support which features is going to lead to building feature-specific kernels whether we like it or not. "If you want to use this new shiny feature, install this other kernel!" One kernel for the debugger, one kernel for matplotlib magics, maybe another kernel for RTC, another kernel for the next feature. I think everyone wants the debugger, it's just the extra externalities of building kernel-specific features into the mainline that could be problematic.

Carreau commented 4 years ago

as a 3.0 feature that only appears for users who have a kernel

As a question: completely hidden of grayed-out ?

But if i don't install such a kernel because it doesn't work due to missing features, I'd like to not have the debugger assets included in my build.

If it gets merged I think it should be grayed out; so that users can at least discover that there might be debugger option. The assets are likely small, and there are many other options people are not using in JupyterLab. You can still probably rebuild w/o said assets. I don't like having non usable visible options by default, but I think hidden one are worse.

I don't think it is fair to say that we are causing a rift by offering a feature that is standard in IDEs.

It's not a rift because of the debugger UI itself, it's a rift as I'm afraid you'll end up with two slightly incompatible types of Python notebooks, and the complexity of understanding that you need to get an extra kernel and share notebook that won't really work. Heck even GvR was confused as to why Jupyter need kernels in a recent discussion and why it had to be installed separately and registered to work (PR about binder to test Python 3.10 Match expression). It ends up badly reflecting on Jupyter because it does not work "out of the box", adn leads to more support request.

One option that was not mentioned is to also make xeus-python a dependency of Jupyter metapackage, and have it installed for (most) users by default. It's ways simpler to tell user "just switch kernel" than go to this long complex speech about what xeus-python is. That's not the best solution IMHO, but I think that's a better one that only merging the debugger UI, announcing it and letting users potentially struggle to figure out why.

A metaphor would be that I'll prefer to build either a full bridge or no bridge at at all, but definitely not 1/2 a bridge.

timkpaine commented 4 years ago

One option that was not mentioned is to also make xeus-python a dependency of Jupyter metapackage, and have it installed for (most) users by default.

Better, but it would still be confusing xpython vs python. Not sure that solves the ux problem sufficiently

afshin commented 4 years ago

We have not yet built the behavior of the debugger front-end when it exists in an environment with no debugger-capable kernels. The behavior we were planning to build was to hide it in order to prevent users from being frustrated by features they cannot use. That seemed like the UX equivalent of: "first, do no harm".

But this user experience is certainly something we can discuss and iterate on.

SylvainCorlay commented 4 years ago

At the moment, installing the debugger extension requires rebuilding the jupyterlab bundle which is where we lose many users, having it by default in the bundle will significantly lower the bar for getting debugging support in jupyterlab (just installing a pip or conda package).

We are currently working with a potension funder to implement the Jupyter debug protocol in ipykernel, and are pretty close to getting it too. Having the front-end already installed will also simplify working with that improved kernel.

The absence of a debugger is one of the main reason why people move to other tools like VSCode, and that is why I think it is important enough to be in the main jupyterlab bundle.

Carreau commented 4 years ago

My vote is to:

echarles commented 4 years ago

Is there a spec documentation on how the debugging protocol behaves (requirements, channels, messages....)? This is needed for others to migrate their kernels.

Also, is there a description of the features that IPykernel is missing to support the debugging protocol?

Carreau commented 4 years ago

see https://blog.jupyter.org/a-visual-debugger-for-jupyter-914e61716559 and https://github.com/jupyter/enhancement-proposals/pull/47

echarles commented 4 years ago

Thx @Carreau. I would say if the debugger goes into core, a core service deserves a core docs. I guess the content of the 2 links you gave could be merged, updated and injected into the existing RTD documentation.

SylvainCorlay commented 4 years ago

Regarding the integration in ipykernel, we propose to do it in two stages:

  1. Implement the kernel messages in the ipykernel control channel

    This will result in a debugging experience in ipykernel, but actions such as e.g. adding a breakpoint will only be executed after the current execution request is completed. This part should be easy, and backward compatible! But the experience will not be 100% ideal because one will not be able to add a breakpoint in the middle of a long-running loop.

  1. Run the ipykernel control channel in its own thread like with xeus-python.

    This may be a simple change - but is more of an unknown because changing the concurrency model of ipykernel can be consequential depending on how people were making assumptions on the current one. Xeus-based kernels support a pluggable concurrency model (control in main thread + shell in separate thread, shell in main thread + control in separate, single event loop).

    We know that only the [shell in main thread + control in separate] case will matter for ipykernel, but that should be in a major release of ipykernel.

Even if we do both (1) an (2) in one go or ipykernel, I think we will want to improve the UI for kernels that only support (1). For example, by having a visual indication that a breakpoint was input, but is not yet added on the kernel side. That will help with kernels providing a naive implementation, and with high-latency scenarios.

Note that we are already making strides on having common code between xeus-python and ipykernel (input preprocessors, display mechanism, magics).

blink1073 commented 4 years ago

IMHO the kernel icon should reflect the language more than the implementation. As as user I just care what language I am running. Right now there is an "optional" feature that is debugging, but that is because it is novel. It will soon be standard, and then as I user I shouldn't need to care "which" Python kernel I am using in the UI, unless I ask for the "about" information.

SylvainCorlay commented 4 years ago

IMHO the kernel icon should reflect the language more than the implementation. As as user I just care what language I am running. Right now there is an "optional" feature that is debugging, but that is because it is novel. It will soon be standard, and then as I user I shouldn't need to care "which" Python kernel I am using in the UI, unless I ask for the "about" information.

I am happy to change the xeus-python kernel icon to be Python. Using the language icon is what we did for xeus-sqlite (SQLite), xeus-cling (C++). It totally makes sense.

blink1073 commented 4 years ago

Note that we pick up ipykernel as part of the notebook dependency or part of the jupyter package. Both conda feedstocks have the same dependency.

SylvainCorlay commented 4 years ago

Note that we pick up ipykernel as part of the notebook dependency or part of the jupyter package. Both conda feedstocks have the same dependency.

Maybe ipykernel should not be listed as a dependency of jupyter_server. That would probably be cleaner for those who want to only use other languages, or who don't want to install a kernel at all in the server environment.

blink1073 commented 4 years ago

That makes sense to me, want to open an issue there?

SylvainCorlay commented 4 years ago

That makes sense to me, want to open an issue there?

Done.

saulshanabrook commented 4 years ago

At the moment, installing the debugger extension requires rebuilding the jupyterlab bundle which is where we lose many users, having it by default in the bundle will significantly lower the bar for getting debugging support in jupyterlab (just installing a pip or conda package).

Just to note, that for 3.0 @blink1073 and @jasongrout are working to switch how we do extensions, so they will be able to be pip/conda installed and not require a rebuild (https://github.com/jupyterlab/jupyterlab/pull/8385, https://github.com/jupyterlab/jupyterlab/issues/7468).

I agree this is a huge hurdle for the current extension experience and will be great to be able to address holistically, for all of them!

blink1073 commented 4 years ago

I just think more divergent kernels is a bad thing. Having to maintain a matrix of which python kernels support which features is going to lead to building feature-specific kernels whether we like it or not. "If you want to use this new shiny feature, install this other kernel!" One kernel for the debugger, one kernel for matplotlib magics, maybe another kernel for RTC, another kernel for the next feature. I think everyone wants the debugger, it's just the extra externalities of building kernel-specific features into the mainline that could be problematic.

In answer to this, xeus-python is inherently at an advantage because it is written in C++, and allows us to do things that are not possible with a Python implementation. Features like debugger and RTC are meant to be available on all kernels. In fact RTC exists at a level above kernels. IMO we should be shoring up any gaps that exist between xeus-python and ipykernel and migrating to use the former.

blink1073 commented 4 years ago

Also note that xeus is a Jupyter project.

echarles commented 4 years ago

Thinking loud, xeus C++ can be a barrier to attract contributors outside of the currenet developer base. The ipykernel developed in Python is better placed to attract contributors (although I read above it is not the case today). Regarding the limits of Python technology, it seems there is a road to overcome those as @SylvainCorlay explains in https://github.com/jupyterlab/team-compass/issues/75#issuecomment-660023960

SylvainCorlay commented 4 years ago

C++ can be a barrier to attract contributors outside of the currenet developer base.

This has really not been our experience. We work on many C++ codebases and recieve numerous high-quality contributions from the community. The ipykernel/IPython combo is a very complex Python codebase (which has this baggage for good historical reasons), and xeus is simpler in my opinion.

Carreau commented 4 years ago

Maybe ipykernel should not be listed as a dependency of jupyter_server. That would probably be cleaner for those who want to only use other languages, or who don't want to install a kernel at all in the server environment.

That was originally put in Jupyter Notebook because a lot of the logic does not support having no kernels available.

MHO the kernel icon should reflect the language more than the implementation. As as user I just care what language I am running. Right now there is an "optional" feature that is debugging, but that is because it is novel. It will soon be standard, and then as I user I shouldn't need to care "which" Python kernel I am using in the UI, unless I ask for the "about" information.

Well, a lot of other things in the kernel depend on which implementation. Xeus support most IPython magics, but IPython has many features that are used by people at the language level:

Autocall (no parenthesis for function call), autoawait, ! shell escape, ?, ??, =! assignment.

Those are non-python syntax, after there are also kernel specific behavior like how to configure and extend.

If you don't know which type of kernel you are running, or just put them under "Python" then you get the bug reports or complaint "The Python kernel does not work". So I think there should be some indication that a kernel is IPython/Xeus, and not just Python. It should of course be relatively clear the language is Python of course, but in the presence of multiple implementation with different behavior, people should know which one they are using.

tgeorgeux commented 4 years ago

I am +1 for bringing the debugger in at 3.0 I am happy to work with the debugger team on the UX of how it should act when xeus-python is not installed.

mlucool commented 4 years ago

I am concerned as an end user and someone who has to support Jupyter for a larger number of people what it will be like to have two Python kernels installed by default. Adding the xeus-python kernel now asks every user of jupyter to go and understand which they should use and when. Users will need to ask themselves which set of features do they care most about. The test matrix will double as well. For example, if I write a custom ipywidget, I now have to test it in two environments (times the number of browsers I support). On the other hand, I want to clearly acknowledge that its important to allow for people to innovate, as xeus-python clearly has done. I am simply saying that Jupyter should not have more than one default kernel for a language because it's not a free lunch - the cost is borne by others.

This I hope gets to the heart of what I believe to be the problem with this proposal. This thread at times seems to be conflating two related issues:

  1. Should we merge the debugger into core
  2. Should we add xeus-python as another preinstalled kernel

I think everyone agrees that if we have a kernel that support the debugger preinstalled, the debugger is a much needed project and adds a lot of value. It's a huge missing feature for lab. The problem is there is only one kernel installed today and it forces us to ask should xeus-python be preinstalled. My personal opinion is that the maintainers of ipykernel/xeus should come up with a proposal for how to handle the two kernel issue and we should make the decision before on that topic we decide what to do with the debugger. Otherwise, to me, it appears the cognitive load added to end users and extra work those of us maintaining jupyter clusters outweighs the benefits that a preinstalled debugger would provide (I also hope that lab@3.x providing a way to just pip install JS reduces the complexity of this install thus reducing the install burden).

SylvainCorlay commented 4 years ago

My take is that we should not require the xeus-python kernel by default (and probably not any kernel by default but that is another discussion). Adding the debugger front-end to lab is not consequential for kernels that don't support debugging yet, and will not change their experience. It will lower the bar to the debugger experience. (In the mean time, VSCode already documents how to debug with xeus-python).

If D.E. Shaw really needs debugging support in ipykernel, they can wait for us to do that work before offering it to their internal users, or even fund us to develop the debugger support in ipykernel and get it quickly. We have a very clear idea of what is required to get there, as we already implemented the backend in xeus-python, and co-developed the front-end.

blink1073 commented 4 years ago

Note also that it is possible to disable a core extension using page_config, something we do at Amazon for a couple of the core extensions that we provide alternate implementations for.

jtpio commented 4 years ago

Merging into core would also increase the visibility of the project and attract contributors who would like to improve the debugger frontend too.

Is there a spec documentation on how the debugging protocol behaves (requirements, channels, messages....)?

The blog post and the JEP posted in the previous comments are indeed good resources for this.

There is also this issue in the @jupyterlab/debugger repo, that we plan to improve and turn into proper documentation: https://github.com/jupyterlab/debugger/issues/64

By merging the debugger extension into core, we can then add the Debug Protocol documentation to the main JupyterLab documentation. This will increase visibility and make it easier to find. The documentation can also serve as the reference point for kernel authors who want to add support for debugging to their kernels.

echarles commented 4 years ago

The blog post and the JEP posted in the previous comments are indeed good resources for this.

There is also this issue in the @jupyterlab/debugger repo, that we plan to improve and turn into proper documentation: jupyterlab/debugger#64

By merging the debugger extension into core, we can then add the Debug Protocol documentation to the main JupyterLab documentation. This will increase visibility and make it easier to find. The documentation can also serve as the reference point for kernel authors who want to add support for debugging to their kernels.

Thx @jtpio to complement the information given by @Carreau. One of the important point IMHO is to consolidate those various sources of information into a proper doc. As you suggest, a first step would be to host the Debug Protocol it on JupyterLab.

Once enough polished, a second step would be to move that to a pure kernel repo. The first candidate I see would be to move it to jupyter-client, on a page like https://jupyter-client.readthedocs.io/en/stable/messaging.html.

I am not sure if we need to doc before a potential merge or if is is fine to leave that for after a merge. I tend to prefer before if possible.

SylvainCorlay commented 4 years ago

One of the important point IMHO is to consolidate those various sources of information into a proper doc. [...] on a page like https://jupyter-client.readthedocs.io/en/stable/messaging.html.

Absolutely, the plan is to add the new messages to the messaging spec documentation.

jtpio commented 4 years ago

Having an entrypoint in the JupyterLab documentation would be useful since this is where the UI is.

Then yes it makes sense to keep the lower-level details in a repo such as jupyter-client :+1:

Carreau commented 4 years ago

I think there are still some question to resolve as to how/how much we advertise that, but I think we can move on with getting the UI code in. It can still be refined later on anyway if needs be.

saulshanabrook commented 4 years ago

I appreciate being able to have the discussion here around this!

It seems like we are mostly in consensus that merging in now is best, so I am in support of that.

The next longer term step after that would be figuring out how to make the kernel we ship by default works with the debugger, which it sounds like we have multiple paths to achieve that we can work towards.

ghost commented 4 years ago

If you put it in the default UI, will it entice kernel maintainers to adopt the debugging functionality?

Otherwise, you could just indicate that the kernel does not support the debugger.

tgeorgeux commented 4 years ago

We can also give a spot in the new Settings UI (if it gets into 3.0) with a spot to enable/disable the debugger with a nice warning that it requires a supported kernel.