Raw kernel auto start fix + #12337
Conversation
|
Testing for this is covered by the ipywidget real python tests, which we know since those tests breaking exposed this issue :). |
|
A bit of explanation for this fix. As of about a week ago (at the bug bash) we realized that raw kernel wasn't correctly honoring the disableJupyterAutoStart setting which should keep our UI from automatically connecting to a kernel when you open a notebook. A fix when in for that, but it broke the nightly real jupyter widget tests. The reason for this break was that due to the change to honor disableJupyterAutoStart we now would fail to auto start in one specific case. This change is to fix that case and also a code health improvement in that area. |
| // When a server starts, make sure we create a notebook if the server matches | ||
| jupyterExecution.serverStarted(this.checkForNotebookProviderConnection.bind(this)); | ||
| // When a notebook provider first makes its connection check it to see if we should create a notebook | ||
| this.disposables.push(notebookProvider.onConnectionMade(this.checkForNotebookProviderConnection.bind(this))); |
There was a problem hiding this comment.
This change here is the crux of the fix. Our autostart preload is handled in serverLoad.ts for two main cases.
- At activation if Interactive window or notebook editor has been used in the last 7 days
- At notebook editor load
After my fix to honor the disableJupyterAutoStart setting raw kernel was respecting case #1 above (which is why we were not initially seeing the issue, for all of us #1 would apply basically all of the time) but we were not honoring #2. Case two is handled by this change here.
But more changes here were for code health cleanup as well. The interactiveBase is supposed to be communicating the the notebook provider for getting a notebook and associated info. It's very crummy coupling for it to have awareness of jupyterExecution and to be referencing it. So in addition to this change I've also made changes to remove jupyterExecution fully from interactiveBase.
| export class JupyterExecutionBase implements IJupyterExecution { | ||
| private usablePythonInterpreter: PythonInterpreter | undefined; | ||
| private eventEmitter: EventEmitter<void> = new EventEmitter<void>(); | ||
| private startedEmitter: EventEmitter<INotebookServerOptions> = new EventEmitter<INotebookServerOptions>(); |
There was a problem hiding this comment.
In interactiveBase.ts you'll notice that I also removed a usage of jupyterExecution that was watching this event. Turns out this event has never (or at least when I went back a year in the code) been hooked up. It's here, but it's never having .fire() called on it. So I removed it and the associated code in a couple of different places.
This is part of LiveShare, but I checked that the live share tests passed, and as this has never been hooked up I feel ok removing it.
|
Kudos, SonarCloud Quality Gate passed!
|
|
Running a flake set to check results: |
For #12330
package-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).