Fix #10733. Support interactive window restore on window reload #10785

rebornix · 2022-07-12T22:44:32Z

With changes in this PR and microsoft/vscode#154974, users can now choose if they want to keep the interactive window on window reloads. Jupyter will now cache the interactive windows and validate them with open tabs (through the new Tab api).

This PR works when if microsoft/vscode#154974 is merged late. If VS Code part is released first, since it's turned off by default, existing users won't see any difference. We could turn the hot exit setting default value to true once we release changes in both products and feel comfortable of doing that.

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
Title summarizes what is changing.
Has a news entry file (remember to thank yourself!).
Appropriate comments and documentation strings in the code.
Has sufficient logging.
Has telemetry for feature-requests.
Unit tests & system/integration tests are added/updated.
Test plan is updated as appropriate.
package-lock.json has been regenerated by running npm install (if dependencies have changed).

src/interactive-window/interactiveWindow.ts

DonJayamanne · 2022-07-12T22:55:20Z

src/interactive-window/interactiveWindowProvider.ts

+            // no need to update it anymore.
+            await result.start(undefined);
+        } else {
+            const preferredController = connection


I think we should cache the controller id as well.
Assume user starts IW, and then changes the kernel, now they reload VS Code, the IW will point to the wrong kernel.

Good catch

changes the kernel, now they reload VS Code

To double confirm, there is no "cell execution" between these two steps, right? And since there is no execution, we don't remember the kernel being used last time, so after window reloading, we still suggest previous kernel.

Cell execution happens after the window is created, so yeah there shouldn't be any cell execution between these two steps.

src/interactive-window/interactiveWindowProvider.ts

src/interactive-window/types.ts

DonJayamanne · 2022-07-12T23:04:03Z

src/interactive-window/interactiveWindowProvider.ts

+                ? this.controllerRegistration.get(connection, InteractiveWindowView)
+                : await this.controllerDefaultService.computeDefaultController(resource, InteractiveWindowView);
+
+            await result.start(preferredController);


I think we should let the create method figure out the preferred controller and pass that into the contructor.
that way we don't have two ways of starting the IW with & without arguments for the notebook controller.
Also cleaner, else the controller can be changed from outside, when i think it should be a value passed into the ctor at the point of creating the IW.

Basically i think we can move this preferedController = connection... code into the create method & then remove the public method start

+1, agree with Don. Controller in the constructor makes the logic simpler in the class itself.

I think we should let the create method figure out the preferred controller and pass that into the contructor.
that way we don't have two ways of starting the IW with & without arguments for the notebook controller

This is what InteractiveWindowProvider#create is already doing now, it creates controller based on connection first and then create InteractiveWindow objects. While on window reload, we create InteractiveWindow from cache, at which point we are not in this code path of InteractiveWindowProvider#create but in a get path.

DonJayamanne

I'd consider moving the code to figure out the controller into the create method as that's where we already do the dterminination, this also removes the need for the new start method (which feels weird, as it can be called with and without an arument)

codecov-commenter · 2022-07-12T23:15:14Z

Codecov Report

Merging #10785 (0299a7e) into main (440397a) will decrease coverage by 8%.
The diff coverage is 77%.

@@           Coverage Diff            @@
##            main   #10785     +/-   ##
========================================
- Coverage     71%      62%     -9%     
========================================
  Files        472      476      +4     
  Lines      27993    33107   +5114     
  Branches    4691     5399    +708     
========================================
+ Hits       19946    20779    +833     
- Misses      6179    10285   +4106     
- Partials    1868     2043    +175

Impacted Files	Coverage Δ
src/interactive-window/types.ts	`100% <ø> (ø)`
...rc/interactive-window/interactiveWindowProvider.ts	`80% <74%> (+2%)`	⬆️
src/interactive-window/interactiveWindow.ts	`69% <76%> (-6%)`	⬇️
src/interactive-window/helpers.ts	`100% <100%> (ø)`
src/notebooks/controllers/liveKernelSwitcher.ts	`60% <0%> (-27%)`	⬇️
src/platform/common/platform/fileSystem.ts	`43% <0%> (-20%)`	⬇️
src/notebooks/controllers/kernelSelector.ts	`28% <0%> (-15%)`	⬇️
src/kernels/variables/pythonVariableRequester.ts	`55% <0%> (-15%)`	⬇️
src/platform/common/platform/fileSystem.node.ts	`70% <0%> (-13%)`	⬇️
src/notebooks/controllers/kernelConnector.ts	`71% <0%> (-12%)`	⬇️
... and 120 more

rchiodo · 2022-07-12T23:19:58Z

src/interactive-window/interactiveWindowProvider.ts

+        window.tabGroups.all.forEach((group) => {
+            group.tabs.forEach((tab) => {
+                if (isInteractiveInputTab(tab) && tab.input.uri) {
+                    interactiveWindowMapping.set(tab.input.uri.path, tab);


I think we have something like getkey for a uri? Would that be better than path? (Path wouldn't match say if the cases were different on windows)

@rchiodo I pushed a fix to use uri.toString().

Is that the same as getComparisonKey?

vscode-jupyter/src/platform/vscode-path/resources.ts

Line 57 in acb70fa

getComparisonKey(uri: URI, ignoreFragment?: boolean): string;

If we need to compare Uris we should use getComparisonKey which can normalize paths, here we are using it as an identifier so a toString() is sufficient.

amunger · 2022-07-12T23:28:09Z

src/interactive-window/interactiveWindow.ts

-        private _owner: Resource,
-        private mode: InteractiveWindowMode,
-        private readonly exportDialog: IExportDialog,
-        private readonly notebookControllerSelection: IControllerSelection,
        private readonly serviceContainer: IServiceContainer,


I'm not a fan of passing the serviceContainer to constructors since it makes the class more coupled with the specific IoC container, but I suppose it's ok since it was already being passed in anyway

Agree, I'm not a fan either. My motivation is avoiding doing serviceContainer.get<> in multiple places where we call InteractiveWindow#ctor.

@DonJayamanne pointed out that it's explicit to us how many services external InteractiveWindow import but moving serviceContainer.get<> into ctor kinds of hiding it. We might want to revisit this along with other challenges I ran into (will leave a comment about this soon)

rebornix · 2022-07-13T01:01:05Z

Synced with @DonJayamanne offline to touch base on #10785 (comment), there are few things mixed together making it a bit complex to understand:

Firstly we want the InteractiveWindowProvider#create to be synchronous (as requested in the comment

vscode-jupyter/src/interactive-window/interactiveWindowProvider.ts

Line 185 in b8f5974

// Note to future devs: this function must be synchronous. Do not await on anything before calling

) when creating InteractiveWindow and inserting it to InteractiveWindowProvider#_windows to ensure that when the IW creation is called fast, we don't run into race conditions
- 🐛 Though currently the race condition can happen as it's asynchronous to fetch default preferred controller
  
  vscode-jupyter/src/interactive-window/interactiveWindowProvider.ts
  
  Lines 201 to 202 in b8f5974
  
  : await this.controllerDefaultService.computeDefaultController(resource, InteractiveWindowView);
  
  and create IW editor
  
  vscode-jupyter/src/interactive-window/interactiveWindowProvider.ts
  
  Line 204 in b8f5974
  
  const [inputUri, editor] = await this.createEditor(preferredController, resource, mode, commandManager);
Thus currently the InteractiveWindow creation is async when being newly created (we pass controller, notebook editor, and input box uri to the constructor), however on window reload, we are restoring the windows synchronously.
- When we restore InteractiveWindowProvider#_windows from cache, the controllers might not be all registered yet (@DonJayamanne pointed out that we are not caching controllers now), so caching controller ID does not always work as we might not have the registered controller yet. preferredController needs to be found later when users click on Run Cell
  codelens, that's the reason why we need to restore https://github.com/microsoft/vscode-jupyter/pull/10785/files#diff-ea1d2b78c1f2061826843821490a1383de7feb165ed7cc45a59efa17809852d9R166
- On Window reload, NotebookEditor might not be available either. In IW creation code path, we always create the IW first (and await for it) so we have the NotebookEditor object before ctor. However, on window reload, an interactive window tab might be hidden (behind other tabs). NotebookEditor is only resolved when we bring it to the front of the workspace, we can't do that right after the window reload since it can break user's current workspace state.
  - That's why we postpone it also to when users click the code lens

It would involve quite some code changes to get this right so I'd love to have them resolved in follow up PRs. One solution we discussed is

Create InteractiveWindow immediately with owner and mode, push it to _windows in both cases (create or restore)
Have one explicit start method to get the InteractiveWindow prepared for cell execution, which includes
- If it's new IW
  - Get preferredController
  - Call interactive.open to create the IW editor in VS Code
  - Start kernel
- If it's a restored IW, we then have the InteractiveTab
  - Reveal the tab into view (currently we use openNotebookDocument API, in the future we would have Tab API to archive this)
  - Get preferredController and set it as the active controller (maybe we don't need this as the notebook controller provider already tells VS Code which controller to use)
  - Start kernel

@rchiodo @IanMatthewHuff @amunger would love to have your insights into this.

rchiodo · 2022-07-13T16:08:35Z

I'm not sure why create needs to be synchronous? Because the user might try to create two interactive windows at the same time? It seems the comment there has already been ignored.

The problem I believe Don was trying to workaround was inside the IW class itself. If the object isn't created with a preferred controller, it then needs to do a lot of stuff async. It felt to me like async creation was better than making parts of IW async.

IanMatthewHuff · 2022-07-13T20:02:23Z

@rebornix The new proposal in your last comment felt pretty legit to me (sorry, just catching up on reviews now). Per this comment it does seem that restore would not need to calculate a kernel:

Get preferredController and set it as the active controller (maybe we don't need this as the notebook controller provider already tells VS Code which controller to use)

Restore feels like a case where it would just handle the kernel via VS Code's controller selection memory. Calculating a preferred feels like a step that only the new case would handle.

rchiodo · 2022-07-13T22:22:05Z

src/interactive-window/interactiveWindowProvider.ts

@@ -132,6 +156,14 @@ export class InteractiveWindowProvider
        if (!result) {
            // No match. Create a new item.
            result = await this.create(resource, mode, connection);
+            // start the kernel
+            result.start();


Would it make sense to just put this into the create?

Fix #10733. Support interactive window restore on window reload

e98f5d7

rebornix requested a review from a team as a code owner July 12, 2022 22:44

rebornix self-assigned this Jul 12, 2022

rchiodo reviewed Jul 12, 2022

View reviewed changes

src/interactive-window/interactiveWindow.ts Show resolved Hide resolved

DonJayamanne reviewed Jul 12, 2022

View reviewed changes

src/interactive-window/interactiveWindowProvider.ts Show resolved Hide resolved

DonJayamanne reviewed Jul 12, 2022

View reviewed changes

src/interactive-window/types.ts Outdated Show resolved Hide resolved

DonJayamanne reviewed Jul 12, 2022

View reviewed changes

DonJayamanne requested changes Jul 12, 2022

View reviewed changes

Use string instead of UriComponents

54a0e44

rchiodo reviewed Jul 12, 2022

View reviewed changes

amunger reviewed Jul 12, 2022

View reviewed changes

separate start and restore.

b8f5974

rebornix requested a review from DonJayamanne July 13, 2022 00:27

fix creation notebook editor assignment

0299a7e

DonJayamanne approved these changes Jul 13, 2022

View reviewed changes

rchiodo reviewed Jul 13, 2022

View reviewed changes

rchiodo approved these changes Jul 13, 2022

View reviewed changes

amunger approved these changes Jul 13, 2022

View reviewed changes

IanMatthewHuff approved these changes Jul 14, 2022

View reviewed changes

rebornix merged commit 31c561d into main Jul 14, 2022

rebornix deleted the rebornix/iw-hotexit branch July 14, 2022 18:03

rebornix mentioned this pull request Jul 14, 2022

Interactive Window creation and restore #10808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #10733. Support interactive window restore on window reload #10785

Fix #10733. Support interactive window restore on window reload #10785

rebornix commented Jul 12, 2022 •

edited

Loading

DonJayamanne Jul 12, 2022

rebornix Jul 12, 2022 •

edited

Loading

rchiodo Jul 12, 2022

DonJayamanne Jul 12, 2022

rchiodo Jul 12, 2022

rebornix Jul 12, 2022

DonJayamanne left a comment

codecov-commenter commented Jul 12, 2022 •

edited

Loading

rchiodo Jul 12, 2022

rebornix Jul 12, 2022

rchiodo Jul 12, 2022

rebornix Jul 13, 2022

amunger Jul 12, 2022

rebornix Jul 13, 2022

rebornix commented Jul 13, 2022

rchiodo commented Jul 13, 2022

IanMatthewHuff commented Jul 13, 2022

rchiodo Jul 13, 2022

Fix #10733. Support interactive window restore on window reload #10785

Fix #10733. Support interactive window restore on window reload #10785

Conversation

rebornix commented Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

rebornix Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 12, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebornix commented Jul 13, 2022

rchiodo commented Jul 13, 2022

IanMatthewHuff commented Jul 13, 2022

Choose a reason for hiding this comment

rebornix commented Jul 12, 2022 •

edited

Loading

rebornix Jul 12, 2022 •

edited

Loading

codecov-commenter commented Jul 12, 2022 •

edited

Loading