Implemented Restart and debug

[JohanMabille]

@jtpio @fcollonval There is still an issue when restoring the degger state, I think it is because we don't stop the debugger before restarting the kernel and the frontend is in an inconsistent state.

The issue is that the new breakpoints sourcePath is different because the pid (which is used in the sourcePath) of the new kernel is different from that of the older kernel.

New menu entry:

[jupyterlab-dev-mode]

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link:

[JohanMabille]

It almost works, breakpoints are correctly set in the back end, and the code stops on the breakpoint. Last issue is breakpoints do not appear on the UI, and the debugger button is not toggled correctly.

[jtpio]

Thanks Johan!

Mind posting a quick screencast to get an idea of the flow and UX?

[JohanMabille]

[JohanMabille]

I will improve that so that the command can be easily added to the toolbar and context menus.

[JohanMabille]

This is my main concern, I had to pass the _update method to public and call it after restoring the debugger state, otherwise the toggle button is switched off and the breakpoints are not restored in the UI. THey are correclty sent to the kernel though, since the code breaks as expected.

We don't want to call the publid method update since all the signal handlers for different channels and other stuffs restored in this method are already there. So I wonder if we should add a special entry for the restart and debug feature to the handler, or simply pass this method in public.

[JohanMabille]

See my comment on index.ts changes above.

[krassowski]

Would it be possible to disable the built-in "restart and ..." commands and overwrite those with the debugger-aware wrappers (instead of adding another "restart and ..." command)? Also cross-referencing #10090.

Edit: maybe disabling commands is not something that can be easily done, so I should have rather said "disable the default menu entries". Of course, this might be a bad idea.

[krassowski]

Or maybe the @jupyterlab/debugger could be an optional dependency of @jupyterlab/notebook-extension as suggested in #10090 (comment) and this behaviour could be added to the existing commands instead?

[JohanMabille]

@krassowska I agree that having an additional "Restart and ...." command is not ideal but it is still better than having the notebook package depending on the debugger package.

@fcollonval has an idea to override the behavior of "Restart and run all" when the debugger is available, but he needs to prototype it.

Another solution would be to add this command as a button in the toolbar and avoid adding it in the menu.

[krassowski]

I thought that having it as an optional dependency is fine; it could be restricted to import type so there would be no runtime dependency.

Ok, yet another idea: could we consider having a callback in the notebook commands saying "kernel will restart" (which will await for all promises returned by callbacks) so we can save the breakpoints, and then after kernel restarted another callback saying "kernel is ready (do you need to do something before running cells?)" (upon which the breakpoints would be restored)?

[JohanMabille]

I think it's more or loess what @fcollonval has in mind. I let him comment for giving the detail.

[fcollonval]

I tested the ability to override the semantic menu by adding those lines in the activate function of '@jupyterlab/debugger-extension:notebooks':

    // As the notebook tracker is required for this, the semantic menu should exists.
    const notebookCmds = Array.from(mainMenu.kernelMenu.kernelUsers).find(
      value => value.tracker === notebookTracker
    );

    if (notebookCmds) {
      // const defaultCommands = { ...notebookCmds };
      notebookCmds.restartKernel = async widget => {
        const state = service.getDebuggerState();
        console.log(state.cells);
        const panel = widget as NotebookPanel;
        const { context, content } = panel;

        await service.stop();
        const restarted = await sessionContextDialogs!.restart(
          context.sessionContext
        );
        if (restarted) {
          await service.restoreDebuggerState(state);
          await handler._update(panel, panel.sessionContext.session);
          await NotebookActions.runAll(content, context.sessionContext);
        }

        return restarted;
      };
    }

This works. But the major problem is the multiplication of commands that do a restart (and something else). So that won't allow to get a consistent behavior.

FYI - restart commands are in 3 places:

• notebook-extension - direct commands:

  jupyterlab/packages/notebook-extension/src/index.ts

  Lines 1502 to 1560 in 29aa914

  	commands.addCommand(CommandIDs.restartClear, {
  	label: trans.__('Restart Kernel and Clear All Outputs…'),
  	execute: args => {
  	const current = getCurrent(tracker, shell, args);
  	if (current) {
  	const { content, sessionContext } = current;
  	return sessionDialogs!.restart(sessionContext, translator).then(() => {
  	NotebookActions.clearAllOutputs(content);
  	});
  	}
  	},
  	isEnabled
  	});
  	commands.addCommand(CommandIDs.restartAndRunToSelected, {
  	label: trans.__('Restart Kernel and Run up to Selected Cell…'),
  	execute: args => {
  	const current = getCurrent(tracker, shell, args);
  	if (current) {
  	const { context, content } = current;
  	return sessionDialogs!
  	.restart(current.sessionContext, translator)
  	.then(restarted => {
  	if (restarted) {
  	void NotebookActions.runAllAbove(
  	content,
  	context.sessionContext
  	).then(executed => {
  	if (executed || content.activeCellIndex === 0) {
  	void NotebookActions.run(content, context.sessionContext);
  	}
  	});
  	}
  	});
  	}
  	},
  	isEnabled: isEnabledAndSingleSelected
  	});
  	commands.addCommand(CommandIDs.restartRunAll, {
  	label: trans.__('Restart Kernel and Run All Cells…'),
  	execute: args => {
  	const current = getCurrent(tracker, shell, args);
  	if (current) {
  	const { context, content, sessionContext } = current;
  	return sessionDialogs!
  	.restart(sessionContext, translator)
  	.then(restarted => {
  	if (restarted) {
  	void NotebookActions.runAll(content, context.sessionContext);
  	}
  	return restarted;
  	});
  	}
  	},
  	isEnabled
  	});

• notebook-extension - kernel semantic commands:

  jupyterlab/packages/notebook-extension/src/index.ts

  Lines 2366 to 2368 in 29aa914

  	restartKernel: current =>
  	sessionDialogs!.restart(current.sessionContext, translator),
  	restartKernelAndClear: current => {

• notebook-extension - run semantic commands:

  jupyterlab/packages/notebook-extension/src/index.ts

  Line 2426 in 29aa914

  restartAndRunAll: current => {

A refactoring of those code to use consistently a restart command then do something else would be good.

Then there is still the trouble of providing a mechanism to carry pre-/post-actions on some commands (in particular the restart command) to achieve a better API to avoid something like the hack above. The discussion should probably be broaden to address for example the need to have a different action on the notebook play and stop toolbar buttons when the user is actually debugging.

[fcollonval]

I moved the menu entry in the settings to be consistent with the new mechanism allowing customization and I rebased the PR.

I only have one comment on missing documentation for new public API. Otherwise, I would suggest merging this first version. And improve it in a second time when hooks on the kernel machinery will be available.

[afshin]

The pattern we used for the "copy shareable link" command in the file browser's context menu was to create a small targeted plugin that adds just that one command (this pattern could be used for a logical grouping of commands) and to have extension authors replace that extension with their own if they need: https://github.com/jupyterlab/jupyterlab/blob/master/packages/filebrowser-extension/src/index.ts#L446-L495

The downside of this approach is that it requires disabling an extension explicitly. But we could instead have that extension expose the disposable object that addCommand(...) returns so that an extension could both know if those commands have already been disposed (i.e. isDisposed) and if they still exist, the author could dispose them and add replacements? We'd have to think through the API a little bit, but it does seem like some of the notebook actions are logical extension points.

[afshin]

Another pattern that came out of conversation with @SylvainCorlay is to have a meta command that makes a decision about whether to use the default command or a debugger version of the command, potentially by always running the debugger version of the command and expecting some return value e.g., true or false to decide whether to short-circuit or to continue and use the default command.

[meeseeksmachine]

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/detect-kernel-restart-inside-extension/8970/3

[fcollonval]

we could instead have that extension expose the disposable object that addCommand(...) returns so that an extension could both know if those commands have already been disposed (i.e. isDisposed) and if they still exist, the author could dispose them and add replacements? We'd have to think through the API a little bit, but it does seem like some of the notebook actions are logical extension points.

For this particular case, a downside of disposing the original command is the need to duplicate/know its code as it may still be executed.

@afshin what do you think about this proposal?

[fcollonval]

Kicking CI

[jtpio]

A follow-up for this could be to try to remove the non-null assertion, since IDebugger.ISources might not be available.

[blink1073]

@meeseeksdev please backport to 3.2.x

[lumberbot-app]

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

$ git checkout 3.2.x
$ git pull

2. Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 c490c1eccdefdc0e8d19e5a77cffd91d2ad79c40

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #10462: Implemented Restart and debug'

4. Push to a named branch :

git push YOURFORK 3.2.x:auto-backport-of-pr-10462-on-3.2.x

5. Create a PR against branch 3.2.x, I would have named this PR:

"Backport PR #10462 on branch 3.2.x (Implemented Restart and debug)"

And apply the correct labels and milestones.

Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove Still Needs Manual Backport label once the PR gets merged.

If these instruction are inaccurate, feel free to suggest an improvement.